-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add config option to control the hamburger menu #6174
Conversation
71667a1
to
2fa3ef3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
let showDevelopmentMenu: boolean | ||
if (props.toolbarMode == Config.ToolbarMode.DEVELOPER) { | ||
showDevelopmentMenu = true | ||
} else if ( | ||
props.toolbarMode == Config.ToolbarMode.VIEWER || | ||
props.toolbarMode == Config.ToolbarMode.MINIMAL | ||
) { | ||
showDevelopmentMenu = false | ||
} else { | ||
showDevelopmentMenu = hostIsOwner || isLocalhost() | ||
} | ||
|
||
if (menuItems.length == 0 && !showDevelopmentMenu) { | ||
// Don't show an empty menu. | ||
return <></> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
every time you use let
a panda is crying.
function showDeveloperMenu(){
if (props.toolbarMode == Config.ToolbarMode.DEVELOPER) {
return true
}
if (
props.toolbarMode == Config.ToolbarMode.VIEWER ||
props.toolbarMode == Config.ToolbarMode.MINIMAL
) {
return false;
}
return hostIsOwner || isLocalhost()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for suggestion.
I added two commits to my branch. One creates functions to avoid creating let
variable, and the second refactors the code a bit more so that we don't need to calculate the menu items when we know the menu is hidden.
@@ -529,7 +554,7 @@ function MainMenu(props: Props): ReactElement { | |||
content={({ close }) => ( | |||
<> | |||
<SubMenu menuItems={menuItems} closeMenu={close} isDevMenu={false} /> | |||
{(hostIsOwner || isLocalhost()) && ( | |||
{showDevelopmentMenu && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
showDeveloperMenu()
def _populate_config_msg(msg: Config) -> None: | ||
msg.gather_usage_stats = config.get_option("browser.gatherUsageStats") | ||
msg.max_cached_message_age = config.get_option("global.maxCachedMessageAge") | ||
msg.mapbox_token = config.get_option("mapbox.token") | ||
msg.allow_run_on_save = config.get_option("server.allowRunOnSave") | ||
msg.hide_top_bar = config.get_option("ui.hideTopBar") | ||
msg.hide_sidebar_nav = config.get_option("ui.hideSidebarNav") | ||
msg.toolbar_mode = _get_toolbar_mode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this one is different than all the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an protobuf enum, not a plain boolean
/string
here, so we need convert config value (string) to enum type.
6598568
to
30c3f6a
Compare
let preferredMenuOrder: any[] | ||
if (props.toolbarMode == Config.ToolbarMode.MINIMAL) { | ||
// If toolbar mode == minimal then show only host menu items if any. | ||
preferredMenuOrder = shouldShowHostMenu ? hostMenuItems : [] | ||
// If the first or last item is a divider, delete it, because | ||
// we don't want to start/end the menu with it. | ||
// TODO(sfc-gh-kbregula): We should use Array#at when supported by | ||
// browsers/cypress or transpilers. | ||
// See: https://github.com/tc39/proposal-relative-indexing-method | ||
if ( | ||
preferredMenuOrder.length > 0 && | ||
preferredMenuOrder[0] == coreMenuItems.DIVIDER | ||
) { | ||
preferredMenuOrder.shift() | ||
} | ||
if ( | ||
preferredMenuOrder.length > 0 && | ||
preferredMenuOrder.at(preferredMenuOrder.length - 1) == | ||
coreMenuItems.DIVIDER | ||
) { | ||
preferredMenuOrder.pop() | ||
} | ||
} else { | ||
preferredMenuOrder = [ | ||
coreMenuItems.rerun, | ||
coreMenuItems.settings, | ||
coreMenuItems.DIVIDER, | ||
coreMenuItems.print, | ||
coreMenuItems.recordScreencast, | ||
coreMenuItems.DIVIDER, | ||
coreMenuItems.report, | ||
coreMenuItems.community, | ||
...(shouldShowHostMenu ? hostMenuItems : [coreMenuItems.DIVIDER]), | ||
coreMenuItems.about, | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I'd extract this to a separate function that computed the preferred order as this logic got more complex.
const showDevelopmentMenu = (() => { | ||
if (props.toolbarMode == Config.ToolbarMode.DEVELOPER) { | ||
return true | ||
} else if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if you return in the upper condition you can replace else if
with if
.
props.toolbarMode == Config.ToolbarMode.MINIMAL | ||
) { | ||
return false | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you don't need else
clause here, you can just write return hostIsOwner || isLocalhost()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this project, I don't think we have one style convention, but if we want to enforce one style, we should do it automatically, e.g. using the ESLint rule.
https://eslint.org/docs/latest/rules/no-else-return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR here: #6205
const devMenuItems: any[] = [] | ||
let devLastMenuItem = null | ||
for (const devMenuItem of preferredDevMenuOrder) { | ||
if (devMenuItem) { | ||
if (devMenuItem !== coreDevMenuItems.DIVIDER) { | ||
if (devLastMenuItem === coreDevMenuItems.DIVIDER) { | ||
devMenuItems.push({ ...devMenuItem, hasDividerAbove: true }) | ||
} else { | ||
devMenuItems.push(devMenuItem) | ||
|
||
if (showDevelopmentMenu) { | ||
const preferredDevMenuOrder: any[] = [ | ||
coreDevMenuItems.developerOptions, | ||
coreDevMenuItems.clearCache, | ||
showDeploy && coreDevMenuItems.deployApp, | ||
] | ||
|
||
let devLastMenuItem = null | ||
|
||
for (const devMenuItem of preferredDevMenuOrder) { | ||
if (devMenuItem) { | ||
if (devMenuItem !== coreDevMenuItems.DIVIDER) { | ||
if (devLastMenuItem === coreDevMenuItems.DIVIDER) { | ||
devMenuItems.push({ ...devMenuItem, hasDividerAbove: true }) | ||
} else { | ||
devMenuItems.push(devMenuItem) | ||
} | ||
} | ||
|
||
devLastMenuItem = devMenuItem | ||
} | ||
} | ||
|
||
devLastMenuItem = devMenuItem | ||
if (devLastMenuItem != null) { | ||
devLastMenuItem.styleProps = lastDevMenuItemStyleProps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this looks big and complex enough to get extracted to a separate function.
5226849
to
52f8796
Compare
3a8df55
to
f8557b8
Compare
frontend/src/App.tsx
Outdated
@@ -713,11 +732,18 @@ export class App extends PureComponent<Props, State> { | |||
RERUN_PROMPT_MODAL_DIALOG && | |||
sessionEvent.type === "scriptChangedOnDisk" | |||
) { | |||
const allowRunOnSave = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a dead code because the variable RERUN_PROMPT_MODAL_DIALOG is always set to False. but I would prefer to deal with this problem in a separate PR, because we have to consider whether we can delete it.
ab2957b
to
3516a09
Compare
479f866
to
b575e46
Compare
Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com>
c9823f5
to
5e3837d
Compare
# By Kamil Breguła # Via GitHub * develop: Add config option to control the hamburger menu (streamlit#6174) Restrict the setting of sensitive options by the CLI flag (streamlit#6376) # Conflicts: # frontend/src/app/App.test.tsx # frontend/src/app/App.tsx # frontend/src/app/components/MainMenu/MainMenu.test.tsx # frontend/src/app/components/MainMenu/MainMenu.tsx
* Add config option to control the hamburger menu * Update lib/streamlit/config.py Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com> * fixup! Add config option to control the hamburger menu --------- Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com>
* Add config option to control the hamburger menu * Update lib/streamlit/config.py Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com> * fixup! Add config option to control the hamburger menu --------- Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com>
* Add config option to control the hamburger menu * Update lib/streamlit/config.py Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com> * fixup! Add config option to control the hamburger menu --------- Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com>
* Add config option to control the hamburger menu * Update lib/streamlit/config.py Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com> * fixup! Add config option to control the hamburger menu --------- Co-authored-by: Snehan Kekre <snehan.minerva@gmail.com>
📚 Context
Now we have a new option
client.toolbarMode
that change the visibility of items in the toolbar, options menu, and settings dialog (top right of the app).Allowed values:
auto
: Show the developer options if the app is accessed through localhost and hide them otherwise.developer
: Show the developer options.viewer
: Hide the developer options.minimal
: Show only options set externally (e.g. through Streamlit Community Cloud) or through st.set_page_config. If there are no options left, hide the menu.Please describe the project or issue background here
What kind of change does this PR introduce?
🧠 Description of Changes
Add bullet points summarizing your changes here
Revised:
Insert screenshot of your updated UI/code here
Current:
Minimal mode with custom items
🧪 Testing Done
🌐 References
Does this depend on other work, documents, or tickets?
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.