fix: Sharedwithyou #14
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @harshitqubits, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces core functionality for handling 'shared' projects, allowing the application to control whether a project is editable by the user. It integrates a new isEditableProject state that influences UI elements, disables interaction with project editing areas for non-editable projects, and adds a dedicated share button.
Highlights
- Project Editability Control: Introduced a new
isEditableProjectstate to manage whether a project can be modified. This state is initialized totrueby default and can be updated via a URL query parameter namededitable. - User Interface Adjustments: The UI now dynamically adapts based on the
isEditableProjectstate. The project name edit icon is hidden, and a new 'Share' button is displayed when a project is not editable. Top navigation icon widths were also adjusted to accommodate these changes. - Interaction Disablement: For non-editable projects, the Blocks, Costumes, and Sounds tabs are visually and functionally disabled, preventing users from making changes to the project content.
- Share Functionality: A new 'Share' button has been added to the top navigation bar. Clicking this button triggers a
handleShareClickevent on aremoteobject, indicating integration with an external sharing mechanism. - Project Saving Logic: The project saving mechanism (
sb3-downloader) has been updated to prevent saving operations for projects that are marked as non-editable (isEditableProjectis true).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces a "Share" button and a concept of project editability controlled by a URL parameter. The changes span across CSS for styling, JSX components for UI, and Redux for state management. The core logic seems to be in place, but there are some important areas for improvement regarding code clarity, maintainability, and production-readiness. My review focuses on removing debug code, simplifying logic, and renaming a confusingly named state variable that impacts readability across multiple files.
| const isprojecteditable = url.get('editable') | ||
| console.log('isprojecteditable', isprojecteditable) | ||
| this.props.setIsEditable(isprojecteditable === 'true' ? true : false) |
There was a problem hiding this comment.
This block has a few areas for improvement to enhance readability and adhere to best practices:
- Debugging Code: A
console.logstatement is present, which should be removed before merging to production. - Naming Convention: The variable
isprojecteditabledoesn't follow the standard camelCase convention. Renaming it toisProjectEditablewould improve readability. - Redundant Logic: The ternary expression
... === 'true' ? true : falseis verbose. The comparisonisProjectEditable === 'true'already evaluates to the desired boolean value.
const isProjectEditable = url.get('editable');
this.props.setIsEditable(isProjectEditable === 'true');
| isPendingState: false, | ||
| notifications: [], | ||
| positionModal: false, | ||
| isEditableProject: true, |
There was a problem hiding this comment.
The property isEditableProject is named in a way that could be misleading. Based on its usage, when it is true, editing capabilities are disabled. This suggests it functions more like a "read-only" flag.
To improve code clarity and maintainability, I strongly suggest renaming this property to something more descriptive, like isReadOnly or isViewOnly. This would involve inverting the boolean's meaning throughout the application (e.g., isReadOnly: false in the initial state), but it would make the logic much easier to understand at a glance.
| </TabList>} | ||
| <TabPanel className={tabClassNames.tabPanel}> | ||
| <Box className={styles.blocksWrapper}> | ||
| <Box className={styles.blocksWrapper} style={isEditableProject ? { pointerEvents: 'none', opacity: 1 } : {}}> |
There was a problem hiding this comment.
The opacity: 1 in the inline style is redundant, as it's the default value for opacity. You can safely remove it to make the code a bit cleaner. This also applies to the same pattern on lines 462 and 465.
<Box className={styles.blocksWrapper} style={isEditableProject ? { pointerEvents: 'none' } : {}}>
| isPendingState: false, | ||
| notifications: [], | ||
| positionModal: false, | ||
| isEditableProject: true, |
There was a problem hiding this comment.
This line has a trailing space and a dangling comment (//). Please remove them for code cleanliness.
| isEditableProject: true, | |
| isEditableProject: true |
2 participants
Resolves
What Github issue does this resolve (please include link)?
Proposed Changes
Describe what this Pull Request does
Reason for Changes
Explain why these changes should be made
Test Coverage
Please show how you have added tests to cover your changes
Browser Coverage
Check the OS/browser combinations tested (At least 2)
Mac
Windows
Chromebook
iPad
Android Tablet