Add group labels to sidebar #547
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
The overall look is great and does help visually navigate that ever growing sidebar My only major concern with this PR is the initial landing page when first launching the app - it's just a blank white screen - and a very long one at that (notice the size of the scroll bar in the screenshot below)
If you want to minimize information display initially presented to the user after launch, I'd at least add the GUIDE logo, or maybe a collage of all the logos, just so it doesn't feel so empty (and/or 'glitchy' - I think most users would expect something to be in the main visual space of an app they just launched - a blank white space is reminiscent of that linux bug where it just fails to render anything that should be there) If you don't care about minimizing initial display of info I'd say just start off on the first tab (Conversions) Also, having the 'settings' pinned as a kind of frozen tab leads to a weird visual effect where it looks like the 'Documentation' (which I'm above proposing rename to 'Help') group has only two tabs, showing the actual third only after scrolling, which by all appearances should have scrolled down past the 'settings' So I'd propose putting 'Settings' under a group 'Configuration' there at the bottom |
|
Alright, I've updated the labels for each of the groups and fixed the initialization so the home page is reached on first load. |
for more information, see https://pre-commit.ci
|
The scrollbar not going down to Settings is good, but I think it is still visually confusing in the default smallish window size. The style is too similar to the menu items above it. And the "Configuration" header for a one item menu seems unnecessary. How about removing "Configuration" and either A) changing the background color of the Settings button or B) using a thicker, darker line to help distinguish the Settings button from the rest of the menu? |
|
A darker background color would be sufficient for me The extra 'Configuration' group is what I was imagining if and only if the group itself is becomes unpinned But if you want to keep it pinned, then yeah, no need for a group label but does need a visual distinguisher from the other sections of the tab |
|
Got it. I've removed the header. Since we don't use a darker gray elsewhere in the application, I've darkened the borders separating the main sidebar body from the header and footer. Is this sufficient? |
|
It looks all right to me. Thanks for the adjustments. Is there a reason the default window size is the size that it is? If we don't plan to add new items to the menu, it would be nice if the default window height were large enough that a scroll bar in the menu is not necessary by default. Maybe 800px instead? https://github.com/NeurodataWithoutBorders/nwb-guide/blob/main/src/main/main.ts#L234-L238 I see that a 1366x768 screen resolution is still fairly common, but I would guess that it is rare among computers that would be working with converting and processing neurophysiology data. |
|
Certainly looks better, but I'd also be in favor of Ryans suggestion if it is possible to force minimal size of window to be large enough to fit the 7 tabs of the app; maybe circle back around to the better solution when/if we ever add more workflows On that note, a mild inconsistency I just noticed; one group is 'Workflow' (singular), other is 'Resources' (plural) - should we make it 'Workflows' to be consistent (and since there is more than one workflow)? |
|
Got it. Added those changes! |
|
While the default window size now starts off as Ryan requested, I am able to manually shrink the window to the previous default size, which is just below where it needs to be to prevent this cutoff If it's possible to constrain the absolute minimal size to above that cutoff that would be great, but let me know if that's too difficult to force |
|
If it doesn’t interfere with functionality, shouldn’t we provide the
flexibility to adjust the window size based on user requirements? While the
sidebar does render differently when smaller, it still functions as usual.
Otherwise I can set the minHeight rather than the height directly on the
browser window instance.
…On Sat, Dec 30, 2023 at 12:58 PM, Cody Baker ***@***.***> wrote:
While the default window size now starts off as Ryan requested, I am able
to manually shrink the window to the previous default size, which is just
below where it needs to be to prevent this cutoff
If it's possible to constrain the absolute minimal size to above that
cutoff that would be great, but let me know if that's too difficult to force
—
Reply to this email directly, view it on GitHub
<#547 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALDAY5JHOCAEH6NOXGGY23LYMBP35AVCNFSM6AAAAABBFWXL36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZSGU4DIMJWGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I'm not saying freeze the entire window size - users can definitely adjust as they desire - simply bump the amount of 'smallest it can be' ever so slightly because the current minimum is just tantalizingly below what would be necessary to avoid the complexity of any scrolling on that subwindow altogether |
|
Got it, makes sense. I wasn’t aware how close the current minimum was to
our desired one.
I’ll make the change when I’m back at my laptop.
…On Sat, Dec 30, 2023 at 2:39 PM, Cody Baker ***@***.***> wrote:
If it doesn’t interfere with functionality, shouldn’t we provide the
flexibility to adjust the window size based on user requirements?
I'm not saying freeze the entire window size - users can definitely adjust
as they desire - simply bump the amount of 'smallest it can be' ever so
slightly because the current minimum is just tantalizingly below what would
be necessary to avoid the complexity of any scrolling on that subwindow
altogether
—
Reply to this email directly, view it on GitHub
<#547 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALDAY5NORA6RQBHR5I6FOKDYMB3YRAVCNFSM6AAAAABBFWXL36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZSGYYDEMJTGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Updated. Didn't catch that SODA initialized the browse window at the minimum size (since they had the same numbers declared twice). I've pulled the |
|
LGTM |
Updated based on @oruebel User Test comments.