-
Notifications
You must be signed in to change notification settings - Fork 34
feat(#3137): add work side menu group #3225
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: dev
Are you sure you want to change the base?
Conversation
fe9bdbf to
33893ef
Compare
b90b656 to
20fdfd2
Compare
564b7ef to
b34d10a
Compare
8013bd7 to
c9c4781
Compare
fe73761 to
24e4427
Compare
24e4427 to
dfbe648
Compare
a07f85e to
9180a16
Compare
|
@bdfranck I know you love conflicts :) |
9180a16 to
e0a159f
Compare
|
@chrisolsen I've resolved the conflicts... for now. 🤪 |
| @@ -0,0 +1,43 @@ | |||
| import { ComponentFixture, TestBed, fakeAsync, tick } from "@angular/core/testing"; | |||
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.
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.
@chrisolsen I've amended a commit to fix that menu item label alignment.
The Account menu is a known issue from the original Work Side Menu PR. (#2937) This is only an issue if teams use the menu in odd layouts, which I don't think we want to promote. I can revisit it if a team finds a valid use case.
I've also fixed the merge conflict.
5612159 to
49c998b
Compare
twjeffery
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.
Looks good @bdfranck, just a few minor things I see:
- the text label alignment issue that Chris mentioned still seems to be there for me (text is slightly down of vertically centering with icons. I like the flex-start, so multi line will align properly a the start, but the line height seems to be shifting the label down too much.
- maybe related to the align-start from #1, the icons all shift slightly up when the menu expands/collapses. removing 270 - align-items: center; seems to fix it.
Screen.Recording.2026-01-19.at.2.36.06.PM.mov
I think this fixes both #1 and #2 above:
A. In WorkSideMenuItem.svelte, remove align-items: center from the collapsed
container query:
@container (max-width: 160px) {
.menu-item {
height: 36px;
margin: 0;
/* remove: align-items: center; */
padding-left: calc(var(--goa-space-xs) + 2px);
}
}
B. In WorkSideMenuItem.svelte, add margin-top to the icon:
goa-icon {
display: var(--goa-work-side-menu-item-icon-display, flex);
margin-top: 2px;
}
C. In WorkSideMenuGroup.svelte, add margin-top to the trailing chevron:
.trailing-icon {
display: flex;
margin-top: 2px;
}
This keeps icon positioning consistent between states (no shift) and aligns
the icons with the text's first baseline.
- The line underneath the leading icon that shows the group is slight right of horizontal center from that icon.
- in Figma we have the parent menu item showing an active state when expanded. I don't know if we need that state to persist, but having the background helps the child items feel closer and related to the parent above them. If we reduce the gap between the parent and the group below it, that could achieve the same thing? I'm curious what you think would be a better design.
- One last minor thing I noticed: in Figma we have the sub items closer to the vertical line (12px), slightly inset from where the parent label starts. Would we want to change this to be closer? Up to you for what the design should be. It feels a bit spaced out when I see it in something like the account menu (see below).
^Figma^
^Code^
Let me know if you have any questions and/or want to discuss anything.
3645332 to
6bc475f
Compare
|
Thanks for the feedback, @twjeffery! I've amended my commit with the following changes:
Anything else? Thanks! |
6bc475f to
2572f8f
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.
Looks great to me now @bdfranck! Thanks for making those changes.
One more thing comes up now that might be a bit confusing:
- when a parent menu item is expanded and the side menu is collapsed, the menu item looks active because of that background. Also, when a user clicks on the parent group when the menu is collapsed, it opens and closes, but the user can't see it because the main side menu is collapsed. See video below:
Screen.Recording.2026-01-20.at.12.21.37.PM.mov
In my latest version of the design system website, I made it so if a side menu group is clicked when the side menu is collapsed, the menu opens automatically. thoughts on adding that approach as applicable here? https://goa-website-generation-v2.netlify.app/
2572f8f to
a1309ca
Compare
a1309ca to
034e8c3
Compare
|
@twjeffery Good catch! I've updated my commit to remove the background when the menu is collapsed. 👍 Instead of expanding the scope of this ticket, I could create a new one for improving the menu collapsed state. Here's all the ideas that I've captured so far:
|
twjeffery
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.
@twjeffery Good catch! I've updated my commit to remove the background when the menu is collapsed. 👍
Instead of expanding the scope of this ticket, I could create a new one for improving the menu collapsed state. Here's all the ideas that I've captured so far:
- Expand the menu when selecting a group
- Expand the menu on load if a sub-menu item is the current item
- Remember the expanded state between pages (and also between sessions?)
- Collapse the menu when reducing the viewport to tablet width
Good idea Benji, approved this version



This PR adds a Work Side Menu Group to use with the Workspace Side Menu. Features include...
Sample code
Sample code
Known issues
This component currently doesn't work with my brittle keyboard navigation in the Work Side Menu. I've created a bug to address it separately: #3279