Skip to content
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

feat(focus): add atomic focus classes #1637

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

dancormier
Copy link
Contributor

@dancormier dancormier commented Feb 6, 2024

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

Copy link

netlify bot commented Feb 6, 2024

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit e0ca975
🔍 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
Copy link
Collaborator

CGuindon commented Feb 7, 2024

How do I test this @dancormier ?

Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

@dancormier The PR looks good to me. Great work!
I would recommend to take the occassion to add a less snapshot test for the misc file similarly to what we do for colors here.

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.

@dancormier
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

giamir commented Feb 7, 2024

@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
Copy link
Contributor Author

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
Copy link
Contributor

giamir commented Feb 13, 2024

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 🙂

@dancormier dancormier marked this pull request as ready for review February 15, 2024 22:32
@dancormier dancormier merged commit 64f0bf2 into develop Feb 15, 2024
10 checks passed
@dancormier dancormier deleted the dcormier/focus-styles-atomic-classes branch February 15, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants