StackExchange / Stacks

Stack Overflow’s Design System
https://stackoverflow.design
MIT License
602 stars 90 forks source link

feat(topbar): add skip link #1692

Closed andrewmeacham closed 2 months ago

andrewmeacham commented 3 months ago

STACKS-584

Adds a "skip to main content" link to the topbar, enabling keyboard users to jump to the main content area.

How to test

Screenshot 2024-04-12 at 13 53 45

Next steps: release a new version of Stacks Classic and add a skip link for the Core topbar(s).

netlify[bot] commented 3 months ago

Deploy Preview for stacks ready!

Name Link
Latest commit 45c8d4729693fccd99936f32e51806814cce8e2f
Latest deploy log https://app.netlify.com/sites/stacks/deploys/661e3b63c5e1a2000889c035
Deploy Preview https://deploy-preview-1692--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 site configuration.

giamir commented 3 months ago

Thanks @andrewmeacham. I also find the color of the "skip to main content" bar a bit too intense but I am happy to go with whatever you designers agree for (as long as it doesn't break or look broken in small viewports). I think the reason why in the deque website the skip to main content bar looks a bit better is because it looks like an alternative bar and not a full width button. If we go with a bar I would personally stay away from the button styles and potentially look into something like our banner styles.

Screenshot 2024-03-26 at 09 29 25
andrewmeacham commented 3 months ago

I would personally stay away from the button styles and potentially look into something like our banner styles.

Great suggestion, @giamir! I will look into the banners.

andrewmeacham commented 3 months ago

Updated to include the banner, @giamir. Is it appropriate to label this with s-banner__skip-link, and should it appear in the banner or topbar documentation?

Screenshot 2024-04-02 at 1 31 24β€―PM
giamir commented 3 months ago

Thanks @andrewmeacham!

The banner looks much better to me now compared to what we had before. I will let @CGuindon to be the ultimate judge of that though. πŸ™‚

_I do believe s-banner__skip-link would the correct modifier name in this context (although I am not so sure if we should associate the skip-link with the banner at all despite using similar styles - see below for more context)._

I am not totally sure where would be the best place to document the skip link. The gov.uk design system documents it as its own component. Would it make sense for us to do the same and create a s-skip-link component? This will give us flexibility on changing the design going forward and a dedicated page to document why this is important at the cost of adding an extra component and potentially duplicate some styles. This will be the usage then:

<a class="s-skip-link" href="#content">Skip to main content</a>

And we will have a https://stackoverflow.design/product/components/skip-link/ page I suppose to document it extensively, mention the relevant WCAG resources, etc... The topbar page could then cross reference this new page to ensure people don't forget about it.

An alternative to this approach would be to couple the skip-link with the topbar a bit more (e.g. creating a s-topbar--skip-link class). If we do this though I would expect the s-topbar--skip-link to be a child element of the s-topbar element - can we do this easily or would we have to change the topbar markup drastically to achieve it? Probably a question for @dancormier. This approach will let us document the skip link in the topbar component page which also makes sense given that I don't expect the skip link to be used often in pages where there is no topbar. Can you think of any scenario/pages where we might want to use the skip-link but not the topbar?

@dancormier @CGuindon @andrewmeacham What is your favourite approach?

  1. Create an independent s-skip-link component
  2. Add the skip-link as a child element of the topbar (s-topbar--skip-link)
  3. Keep things as they are now using a modifier for the banner component (s-banner__skip-link)
  4. Any other idea ???

Personally I am undecided between option 1 and 2. Answering this question Can you think of any scenario/pages where we might want to use the skip-link but not the topbar? might help me to lean more towards one or the other. I would advice against option 3 since while they might share similar styles there is no semantic connection between the banner and skip-link in my opinion.

Sorry @andrewmeacham if this is taking a bit longer than expected but I think we should not rush this decision through and take the time to discuss our options. πŸ™‚

andrewmeacham commented 3 months ago

@giamir I don't foresee it being used without the topbar. I just didn't see a straight-forward path to making it a child-element.

CGuindon commented 3 months ago

@giamir +1 to Drew's comment. I'll defer to @dancormier best judgment but from my perspective, I think #2 (include in top bar) makes the most sense as I can't think of a reason to have this without it.

dancormier commented 3 months ago

Can you think of any scenario/pages where we might want to use the skip-link but not the topbar?

I can think of a situation or two where the topbar wouldn't be appropriate but we might still want a skip link (maybe custom marketing/landing pages come to mind and I'm sure there are others). That said, I think those situations will be rare and we could potentially provide guidance on including skip links outside of the topbar usage.

What is your favourite approach?

  1. Create an independent s-skip-link component

  2. Add the skip-link as a child element of the topbar (s-topbar--skip-link)

  3. Keep things as they are now using a modifier for the banner component (s-banner__skip-link)

  4. Any other idea ???

Nope! I think option 1 or two are the way to go.

Personally I am undecided between option 1 and 2.

I'm also torn between option 1 and 2. I want to ensure consumers of Stacks are encouraged to include a skip link whenever it's necessary.

Pros for standalone skip link

Pros for including a skip link child element within the topbar

I'm leaning lightly towards including the skip link as a child of topbar. If we go this route, I'd recommend we add detail to the accessibility docs to give sans-topbar skip link guidance.

[…] I would advice against option 3 since while they might share similar styles there is no semantic connection between the banner and skip-link in my opinion.

Ditto! I think it's best to strip away the styling of the skip link and think purely in terms of semantics and logical associations when considering where to include it. The banner is only really relevant in terms of styling, so I'd vote against option 3.

giamir commented 3 months ago

It looks like option 2 is the winner then. Here is what we need to do based on what I have read:

@andrewmeacham Would you be comfortable working on the above tasks or would you prefer somebody from Stacks to take over? Let us know. πŸ™‚

andrewmeacham commented 3 months ago

@giamir Yes, I'd very much appreciate if you take it from here. Thank you!

giamir commented 3 months ago

@andrewmeacham No problem. @dancormier and I will look into this and circle back to you when it's ready to be integrated in Core. Thank you for kick starting this.

giamir commented 2 months ago

@dancormier @CGuindon after few ping pongs with Dan this PR should now be ready for a final review before being merged and then integrated in Core topbars too. An "How to test" section is in the description. Thanks in advance for your feedback.

CGuindon commented 2 months ago

For some reason I can't add @andrewmeacham as a reviewer but would like his final eyes on this.

giamir commented 2 months ago

For some reason I can't add @andrewmeacham as a reviewer but would like his final eyes on this.

@andrewmeacham is the creator of this PR so it should get a ping for each discussion we are having. @andrewmeacham could you give us a thumbs up? Then we could proceed with the Core integration next. Thank you. 😊

giamir commented 2 months ago

@andrewmeacham I am going to merge this PR otherwise I am blocked. I will make sure to tag you in the integration PR in Core and if there we find something to adjust back in Stacks Classic we can easily cut a patch.

andrewmeacham commented 2 months ago

Apologies, @giamir. I've been OOO. I'll catch the next one!