-
-
Notifications
You must be signed in to change notification settings - Fork 42
feat: organize TopBar "more" menu into sections #563
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: master
Are you sure you want to change the base?
feat: organize TopBar "more" menu into sections #563
Conversation
|
I see the linting errors, I'll take a look at fixing this up later today. I haven't used dev containers before, but I noticed eslint was not showing errors in the IDE -- I'll try to figure it out myself, but if anyone knows an easy way to fix the configuration locally so I can see the errors without running the full lint script, that would be helpful. |
I think it's possible that something broke with the eslint config at some point. I noticed some issues last week but haven't had a chance to look into it yet. |
|
Sorry for accidentally closing - my comment got cut short. I will try to look into the eslint stuff later today or this weekend if you don't figure it out by then. Overall this looks great - I think the headings are definitely an improvement and I like that it helps organize the code a bit better. I'm not convinced about left justifying everything, but I'm not a designer. I'll have to run it locally so I can play around and see what I think. I'll try to do that ASAP so we can get this merged once the build is clean! Thanks again! |
|
I played around with this a bit locally and it seems to be working just fine. I think that on mobile I much prefer the layout you've come up with (left aligned, tighter spacing)! On desktop it just feels a bit unbalanced with all that whitespace to the right side, though. Do you think you're up for trying to add some margin at the higher break points? |
@mdirolf Sure thing! Thanks for the quick review, I'll let you know if/when I figure out the linting issue |
|
@mdirolf I tried my hand at fixing the eslint config, I used this as a reference. I believe the configuration is essentially unchanged, but you may want to test that all of the rules are configured correctly. I also went ahead and cleaned up the linting errors I introduced. The final commit I added centers the more menu items within the overlay, which I think works for most screen sizes (see attached demo below). Screen.Recording.2026-01-31.at.5.45.05.PM.mov |
mdirolf
left a comment
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.
Overall this looks great! Thank you for sorting out the eslint stuff - that seems to be working again for me with your changes.
I have two general notes in addition to the very minor change I commented on:
-
The CSS changes in TopBar.module.css also affected other places that used the top bar dropdown menus - most notably the more menu on the puzzle solving page. So that appears a bit wonky now (mostly just still left-aligned since it doesn't have the new CSS you added in topbarchildren.module.css). Is there a way we could move the styling you did in
.topBarMoreDropdownup a level so that every <TopBarDropDown...> site-wide gets it? -
This one probably isn't a blocker but I have a slight useability preference for the way that on the current site the hover/click target for buttons on the menu expands to the full width of the modal. I think the centering you did looks nicer, but now the hover target is only the width of the largest button. Is there a way we can keep your nicer aesthetics with the older hover/click target width?
Makes sense to me, I'll take a look.
There are probably a few ways we could do this, I agree it's a nicer UX to have the target span the full width. Another alternative would be to reduce the overlay width to fit the children, although that's a bigger departure from the application's current design language. |
…d left-aligned button content
|
I pushed a commit which addresses the layout by setting a fixed-width on the button content. This was necessary to ensure the content remained left-aligned in the center of the button while having the button span the full width of its parent. In the previous layout I was able to use I think this solution works, but it will require the additional maintenance burden of updating of the button content |
|
This looks good, and I don't mind the add'l maintenance burden, but there are some other I wonder if a better approach would be to make the width a required prop to |
| } | ||
|
|
||
| .topBarPuzzleMoreDropdown { | ||
| --dropdown-col-width: 220px; /* Explicit width so that child content of each button is aligned */ |
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 can make a note to do this after merging if it's annoying, but I think we need to change this to 230px.
There's no way you would've known this but the Slate integration uses a different font and 220 isn't giving it enough space for the enter rebus button.
That sounds good to me, having an explicit requirement is definitely better. I should be able to push an update in the next day or so. |
Summary
This PR organizes the options within the
TopBar"more" menu into logically grouped sections.Implementation Details
<TopBarChildren />componentuseSnackbarhook so that the methods returned had stable references and did not cause extra re-renders when passed as props<TopBarDropdownSection />and<TopBarDropdownSectionHeader />components to support the new sectionsextractscript to update the translation filesCloses #561