-
-
Notifications
You must be signed in to change notification settings - Fork 475
settings: Always show the sidebar in case of settings page. #1431
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
base: main
Are you sure you want to change the base?
Conversation
Fixes zulip#1077. Users were getting stuck in the settings page, unable to click on an organisation tab to go back. Enabling the sidebar for settings page in all cases will help ensure that the users can navigate back when needed.
75e2279
to
d15c50e
Compare
// We do not want to hide the sidebar in case of the settings page. | ||
if (this.tabs[this.activeTabIndex]?.properties?.page === "Settings") { | ||
show = true; | ||
} |
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.
So it appears this is changing the behavior to now write to the settings (the default of whether to show the sidebar on startup) when toggling the sidebar manually. I don't think that's what we were looking to do?
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.
I'm not sure if that is the case i.e. the default of whether to show the sidebar is changed by setConfigItem.
From my understanding, it does not override any global config if override
is not set to true, which it is not in that case.
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.
I'm still confused. Previously, this function, which is called from multiple code paths, did not call setConfigItem
. Now it does. This to me seems like a behavior change that is not part of "Always showing the sidebar in case of settings page".
If this is fixing another issue that you noticed while working on this PR, maybe it should be its own commit.
But to me it looks like it is introducing a bug. I think previously, the behavior was we have a setting for whether the sidebar is hidden in general in the configuration that you access via the settings panel, and also there's a quick way to toggle it without changing what happens on next startup. This seems to make clicking the quick toggle always change it on next startup.
I don't really understand the purpose of adding that setConfigItem
here at all, but that's my understanding of what impacts it has.
Fixes #1077.
Users were getting stuck in the settings page, unable to click on an organisation tab to go back. Enabling the sidebar for settings page in all cases will help ensure that the users can navigate back when needed.
Screenshots and screen captures:
Platforms this PR was tested on:
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: