StackExchange / Stacks

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

feat(focus): add atomic focus classes #1637

Closed dancormier closed 5 months ago

dancormier commented 5 months ago

This PR adds atomic focus style classes to the library. I think this is necessary in order to allow consumers to style elements that otherwise wouldn't receive the desired focus style.

https://deploy-preview-1637--stacks.netlify.app/product/base/interactivity/#focus

netlify[bot] commented 5 months ago

Deploy Preview for stacks ready!

Name Link
Latest commit e0ca9757fd015fda57c3bf2b197ad5f0eaee0395
Latest deploy log https://app.netlify.com/sites/stacks/deploys/65ce90d862322a0008f5944d
Deploy Preview https://deploy-preview-1637--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.

CGuindon commented 5 months ago

How do I test this @dancormier ?

dancormier commented 5 months ago

I was also wondering if we already know that we will have use cases for all the 4 classes in Core. Creating larger API areas = More maintenance efforts. Anyway I am also happy to expose all 4 (actually 8 with the conditional one) at once.

I think these classes will be useful for custom built components within consumer libraries, but I'm not too sure of a concrete example. At first, I was assuming it it would be used for the post summaries that show up as search suggestion but I those can probably be handled with removing .outline-none and adding .s-block-link to those elements.

Eventually, I expect Stacks to handle a[href], button and similar elements (or maybe just *:focus πŸ€”) which I'd expect would reduce or eliminate the value of these utility classes. @giamir Does this require a broader conversation (and a delay on merging)?

dancormier commented 5 months ago

How do I test this @dancormier ?

Sorry, I should have mentioned how this can be tested @CGuindon. You can see these atomic classes in action in the docs at https://deploy-preview-1637--stacks.netlify.app/product/base/interactivity/#focus

giamir commented 5 months ago

@giamir Does this require a broader conversation (and a delay on merging)?

I am a fan of adding things only when we have a real world use case for it. Predicting future needs is difficult so I will hold off from merging those classes until somebody in Core will present us with a use case (or we find one ourselves). πŸ™‚

dancormier commented 5 months ago

Update: I found a use case for these atomic classes! Currently, the editor programmatically applies bs-ring to the editor DOM element (by default, a div) when the editor is focused. Now that we have these new focus styles, we need to programmatically apply focus styling to that element but we currently do not have a convenient way to do so. Atomic focus classes would resolve this issue.

giamir commented 5 months ago

I am happy to merge this in if we have a use case now for them. I only had a minor comment around import statements in the less file. Thanks @dancormier πŸ™‚