Skip to content

Conversation

@giamir
Copy link
Contributor

@giamir giamir commented Nov 12, 2025

SPARK-105

Figma

Consult the new stacks classic component in the preview docs.
And the new stacks svelte component in storybook

Unresolved minor points (for another ticket potentially):

  • Svelte component does not support yet the role tablist and tab, we could iterate on it once there is a concrete use case for. It is also a purely accessibility matter.
  • We might benefit from a decorative-only version of the avatar component to prevent screen readers from reading text twice (in the context of navigation the avatar could be considered decorative I believe similarly to what we do for icons)

Complex example using Svelte in Storybook (inspired by the new SHINE sidebar)
I spent some time putting this example together because it saved us few ping pongs with the engineers trying to implement it and realize things were missing. @CGuindon could you confirm that this is the desired animation behavior we want in the new SHINE sidebar?

Screen.Recording.2025-11-20.at.13.33.59.mov

@changeset-bot
Copy link

changeset-bot bot commented Nov 12, 2025

🦋 Changeset detected

Latest commit: 29efe57

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Nov 12, 2025

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 29efe57
🔍 Latest deploy log https://app.netlify.com/projects/stacks/deploys/692030aef1e9470008e86534
😎 Deploy Preview https://deploy-preview-2043--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link

netlify bot commented Nov 12, 2025

Deploy Preview for stacks-svelte ready!

Name Link
🔨 Latest commit 29efe57
🔍 Latest deploy log https://app.netlify.com/projects/stacks-svelte/deploys/692030ae69c66100089350f4
😎 Deploy Preview https://deploy-preview-2043--stacks-svelte.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@giamir giamir requested a review from dancormier November 17, 2025 18:35
@giamir giamir assigned CGuindon and unassigned CGuindon Nov 17, 2025
@giamir giamir requested a review from CGuindon November 17, 2025 18:35
@CGuindon
Copy link
Collaborator

CGuindon commented Nov 18, 2025

Looking really good! A couple of things that jump out at me seeing this nav in the Stacks docs (small nitpick stuff)

1.Documentation improvements optional
These aren't necessary but would be nice to have I think — these could also live in a different type of page I know @phoebe-leung was exploring around showing examples of "larger components" using all the pieces together. We could skip these for now and I'll document below or my own purposes later:

  • Including a popover with the Dropdown example (feels weird to click the dropdown menu button and nothing happens.
  • Including a button on the right of the dropdown example (like in the Figma with the "Today" button right aligned) — common top bar pattern

  1. New badge We would still want to support the option to have the "New" badge that we do sometimes now. I haven't done badges yet but we should still keep an example (maybe with this one?) that has a "new" badge like the activity indicator and it would update once that component gets completed. I didn't have an example of that in Figma — my fault.
Screenshot 2025-11-17 at 4 28 49 PM

@giamir
Copy link
Contributor Author

giamir commented Nov 18, 2025

Including a button on the right of the dropdown example (like in the Figma with the "Today" button right aligned) — common top bar pattern

I added a tonal button in one of the horizontal examples but it won't look great until the button PR is merged in beta.
Screenshot 2025-11-18 at 10 21 22

New badge We would still want to support the option to have the "New" badge that we do sometimes now. I haven't done badges yet but we should still keep an example (maybe with this one?) that has a "new" badge like the activity indicator and it would update once that component gets completed. I didn't have an example of that in Figma — my fault.

I have added the badge in one of the examples but similarly it won't look good until the badge PR is merged. At the moment the height of the small badge is enough to mess up the 42px overall height of the navigation item containing it. This should not be a problem though with the new badge design that has considerably less height.

Screenshot 2025-11-18 at 10 23 53

Including a popover with the Dropdown example (feels weird to click the dropdown menu button and nothing happens.

I left this out for now since it was working this way also in the previous version of the docs. We can always add it at a later stage

@CGuindon
Copy link
Collaborator

  1. PADDING CHANGE on the selected/hover states
    Can we reduce the padding on the no icons variant (default) to be 6px top/bottom (instead of 8px) but keep the 8px for right/left — that makes a full list of no icon items fit closer together but still gives the selected state some breathing room.
Screenshot 2025-11-18 at 7 04 02 AM

Copy link
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to add my comment through the request change — see above

@giamir
Copy link
Contributor Author

giamir commented Nov 18, 2025

Forgot to add my comment through the request change — see above

@CGuindon Addressed in 3aaf689

@dancormier
Copy link
Contributor

The only thing missing should be updating the dropdown caret (@dc I would appreciate a suggestion from you on how to proceed since the caret changed significantly).

For the caret, I'd suggest using a pseudo-element much like I did on Menu (see 8c4c113).

The gist is that the SVG(s) will need to be hard coded into the Less with a Less variable to define the fill on a per-mode basis. I'd expect something like this:

.s-navigation {
…
    // Less variables for check svg fill color
    @me-bg-image-fill: .set-black()[default];
    @me-bg-image-fill-dark: .set-white()[default];
    @me-bg-image-fill-esc: escape("@{me-bg-image-fill}"); // color escaped for URL usage
    @me-bg-image-fill-dark-esc: escape("@{me-bg-image-fill-dark}"); // color escaped for URL usage
    --_na-after-bg-image: url("data:image/svg+xml… fill='@{me-bg-image-fill-esc}'…");
    
        // CONTEXTUAL STYLES
    .dark-mode({
        --_na-after-bg-image: url("data:image/svg+xml… fill='@{me-bg-image-fill-dark-esc}'…");
    });
…
    & &--item {
        &__dropdown {
            &:after {
                background-image: var(--_na-after-bg-image);
                content: "";
                display: inline-block;
                height: var(--su16);
                margin-left: auto;
                width: var(--su16);
            }
…
        }
    }
}

I know it seems a little weird, but it's the best approach I've found for applying our colors to background SVGs while still supporting our color modes. It's much more reasonable when the color is monochromatic, like it is here. Let me know if this makes sense and if you'd like any more help. Happy to do so 🙂

@giamir
Copy link
Contributor Author

giamir commented Nov 19, 2025

@dancormier for the caret I think I found a solution that could be slightly more elegant (unless I am missing something).
See this commit and let me know your thoughts: 05685a2 🙂

@giamir giamir marked this pull request as ready for review November 20, 2025 12:44
@giamir giamir requested a review from mukunku November 20, 2025 12:47
@giamir giamir requested a review from ttaylor-stack November 21, 2025 09:52
Copy link
Contributor

@dancormier dancormier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here @giamir!

@CGuindon
Copy link
Collaborator

The only thing I noticed was that the background on this bottom part of the documentation example should be white (not grey?) but I'm going to approve anyway since the actual component looks great!

Screenshot 2025-11-21 at 10 02 38 AM

@mukunku mukunku merged commit 6f6d576 into beta Nov 21, 2025
18 checks passed
@mukunku mukunku deleted the SPARK-105/navigation branch November 21, 2025 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants