fix+feat: show sidebar context-menu when it is "narrow"/add narrow mode#4062
fix+feat: show sidebar context-menu when it is "narrow"/add narrow mode#4062arikorn wants to merge 7 commits intobitfocus:mainfrom
Conversation
The recent PR to manage unfolding mode added a "temporary narrow" mode to the sidebar (user clicked in the narrow region and didn't leave the sidebar). This PR fixes it so the user can bring up the context-menu in that mode. bonus: convert chained `:not():not()` to `not(a,b)` lists.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSidebar SCSS selectors were reorganized to make Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Previously, the left border only showed when the group was open, but this nicely highlights the group togglers, especially in the narrow sidebar.
Allow user to keep the sidebar narrow (applies to both regular and mobile mode). This requires only very small tweaks to the existing code. Currently setting the mode is accessible only from the context-menu.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webui/src/Layout/Sidebar.tsx (1)
263-274: Consider hiding the footer fold toggle while persistent narrow mode is active.
narrowModewins overunfoldablehere, so the footer toggle can still flip the stored fold/full-width mode with no visible change. That makes the control feel broken and can surprise users when they later turn off Keep Sidebar Folded.Also applies to: 309-313
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c45a556-a474-41f7-a515-e5b1f6b1e622
📒 Files selected for processing (2)
webui/src/Layout/Sidebar.tsxwebui/src/scss/_layout.scss
- rename `narrow` to `tempNarrow` to make its intent clearer - add function to set it false when entering narrowMode - hide "sidebar-header-toggler" (i.e. foldable-mode toggler) when in narrow mode While the function isn't strictly necessary, it keeps the behavior uniform.
|
The last (two) commits adds a feature allowing the user to keep the sidebar permanently narrow (including in mobile mode: when opened it opens in narrow mode). The feature is currently accessible only from the sidebar context menu. Most of the functionality was already built in, so this is a "trivial" upgrade (and it also tidies up a couple of lines of CSS that weren't needed). |
|
The short answer is that these are all "pre-existing" to this PR, but worth re-examining. They are also all things I've touched in previous PRs. So...
Not guesswork, but you do have to hover for a second to see what the dot represents (i.e. tooltip tells you). A minor problem: currently the tooltip is on the icon itself, so you have to hover precisely... Several options come to mind (in order of my effort needed):
Let me know your choice!
Yeah, this has been around since I "fixed" the expand/collapse behavior on those buttons quite some time ago. Previously it would expand but not collapse until you clicked on the caret itself, which I found annoying and confusing. The problem, of course is that these buttons are serving double-duty of selecting the page and opening the group. Not sure I understand your counterexample though. If you clicked variables to open the variable group, then went to buttons, the variable group is still open in the sidebar, so why would you need to click "variables" again except to close the group? But yes, it does close the group and go to the variables index page. (I suppose it would be nice to be able to click just the caret to open/close the group w/o navigation -- not sure if that's a tricky or simple change.) Options:
Yes, that was in one of my recent sidebar PRs. It think it helps highlight the opened group, but it's definitely not necessary if you don't like it. Just a note, going back to the surfaces toggler: it's what it does anyway "most" of the time, since it and its first child are always lit together! Anyway, I'm happy to turn this off in general (I could probably even turn it off for surfaces by making the "active" state non-fuzzy, though it might be confusing if the use selects the page then closes the group.) So just let me know your choices on all of these! |
Its another workflow to solve navigating these pages which all have the same icon. Instead of spending time trying to find the right circle, I would click on the variables page and use the list it presents to find the page I am after.
actually that raises a bigger issue, in that there is no way to collapse a group other than to go to that page.. so once I leave the variables page the only way to collapse the group is to go back to it. The long list of connections didnt bother me before because when I didnt want to see it I could collapse the variables section and ignore it Hmm.. another bug here, when I refresh the page the 'current' group isn't expanded, making it hard to see where I currently am (without accidentally navigating away)
I think that is fine for this mode, as this is an explicit opt in to keep it collapsed. A slight reshuffle of the content seems reasonable to ensure it makes sense for navigating.
I'm not entirely opposed to this, I did have the same thought the other day, but I'm not 100% sure. That layout won't work for mobile either, but maybe thats fine? I don't think I want a change like this before 4.3 is out though
Thats how it used to behave, it was a split button so separete hover styles. I dont remember if clicking the link portion would expand the group or not
From firing up 4.2, it used to behave that clicking the link toggled the group, but there was a separate button to trigger just the collapse/expand. I would say that clicking the link portion shouldnt ever collapse it (maybe unless it does no navigation?)
The problem with this is that it turns some navigations into having more clicks, especially if groups are auto-collapsing.
I wouldn't mind a subtler style, but as shown in that screenshot the same colour causes it to very much drown out the active indication. |
- restore "split-button" functionality to group togglers - - note that unlike 4.2 and earlier, all groups togglers are split, not just ones that do double-duty (i.e. nav). This maintains consistency in the UI and supports the next feature - Open groups now only highlight the caret unless the nav-path is active. - Clicking on the label of a nav-group will only close it if no navigation happens, i.e. either this is not a nav button or the user is already on that button. - Clicking on the caret toggles the group without navigation - In `narrowMode` a "popover" showing the full name -- or title if present -- appears to the right on hover anywhere in the button or on focus. (The default text for group toggles has the word "group" appended.) Note: while this commit does not directly address the refresh-page issue (refresh-page doesn't preserve group open/closed state) it does allow you to open the group w/o navigating in all modes but narrow. Technical note: I added a new context component to support some of these features. Two alternatives are: (1) use the SidebarStateContext, which would be a little more fiddly and may create extra renders or (2) pass the arg down to the relevant menu-items, which requires changing all menu-item definitions as well as the generic `CNavGroup` definition (which required modification anyway, so maybe not a big deal?) -- may or may not be a better option.
|
The latest commit address (almost) all of your issues, @Julusian. To copy from and add to the commit message:
While this commit does not directly address the refresh-page issue (refresh-page doesn't preserve group open/closed state) it does allow you to open the group w/o navigating in all modes but narrow... My opinion here: Losing state on refresh: (a) it's a feature not a bug -- allows you to reset a (perceived) mess (b) although a simple way to "keep" state would be to open the group if something in it is selected, it would probably then prevent closing the group if the user wanted to close it.
Understood. In this commit I removed the surfaces subpage, since it's just noise and ever-so-slightly confusing. I just realized a minor bug: "surfaces" isn't active when "remote surfaces" is selected. The problem is that getting the "only close if no nav happened" to work requires matching the exact page, so for it to work with surfaces, the group toggler has to be set to
Agreed, but anticipating such a change bolsters the case, in this PR, for not hiding items that don't have an icon. Technical note: I added a new context component to support some of the new features. Two alternatives are:
Let me know if you have an opinion. Otherwise, I'm happy with the current way despite a bit of white-space noise it introduces (i.e indenting the entire return group of
|
|
I wrote:
I think this needs to be done, regardless. The current structure doesn't really reflect the page contents properly (the page is more than just the configured surfaces). I'll post another PR. |



The recently merged PR (#3995) to manage unfolding mode added a "temporary narrow" mode to the sidebar (user clicked in the narrow region and didn't leave the sidebar). This PR fixes it so the user can bring up the context-menu in that mode.
bonus: convert chained
:not():not()to:not(a,b)lists.Summary by CodeRabbit