Customize menubar #4960
Customize menubar #4960
Conversation
rschamp
left a comment
rschamp
left a comment
There was a problem hiding this comment.
Tested it out and it looks good! I think we should use a default prop for logo, but everything else is just a suggestion.
src/components/menu-bar/menu-bar.jsx
Outdated
| })} | ||
| draggable={false} | ||
| src={scratchLogo} | ||
| src={this.props.logo ? this.props.logo : scratchLogo} |
There was a problem hiding this comment.
This looks like scratchLogo should be the default of the logo prop.
src/components/menu-bar/menu-bar.jsx
Outdated
| <MenuItem | ||
| className={className} | ||
| onClick={loadProject} | ||
| onClick={this.handleSaveToComputer(downloadProjectCallback)} |
There was a problem hiding this comment.
Just a note — I know you didn't name this. This method name is misleading since it looks like an error. I would expect onClick="{this.handleSaveToComputer}, i.e. set the onClick handler to that method. But instead this is a function that returns a handler, so the name should probably reflect that.
Show or hide the language and file menus.
Show alternate logo if GUI is initialized with a different file.
chrisgarrity
force-pushed
the
customize-menubar
branch
from
854c4ee to
8d1fb77
Compare
* add scratchLogo as default for logo prop
* prefer `{featureAllowed && (<Component />)} over ternary operator with a null ‘else’
* renamed `handleSaveToComputer` as `getSaveToComputerHandler` to more clearly represent what it’s doing (returning a function to be the handler)
* renamed the parameter that is the function to handle clicking on the load project menu item (also updated the comments in `SBFileUploader`)
chrisgarrity
force-pushed
the
customize-menubar
branch
from
8d1fb77 to
6bf9fac
Compare
|
@chrisgarrity looks like this PR didn't allow these options to be passed in from the container, so this can't be used externally |
|
@paulkaplan aargh - yes, forgot to add the props to the gui container and pass them through. |
|
@paulkaplan on second look, the GUI container passes everything not explicitly called out to the component as |
3 participants
Resolves
In some uses of gui (e.g. ChromeOS/Android), the app will handle changing language and file management. So the menu should be customizable to show or hide the language and file menus along with allowing a gui consumer to provide an alternate logo.
Proposed Changes
Adds
canChangeLanguage,canManageFilesandlogoprops to the gui and menubar. The menus default totrue, and the default scratch-logo is shown if no logo is provided.Test Coverage
Current tests pass. The current menu bar integration test will use the default config. Is there a good way to add a test for the options to hide the menus?
Browser Coverage
Check the OS/browser combinations tested (At least 2)
Mac
Windows
Chromebook
iPad
Android Tablet