Closed dancormier closed 7 months ago
Name | Link |
---|---|
Latest commit | 87577ccd6e3b8911d821a96942b71de0dbda8baa |
Latest deploy log | https://app.netlify.com/sites/stacks/deploys/65b7c81e857d7b00080cb7f8 |
Deploy Preview | https://deploy-preview-1614--stacks.netlify.app |
Preview on mobile | Toggle 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 We could make one of those changes but I wanted to show what they'd look like in practice before I make these changes, plus run an alternative option by you as well.
- Add an extra (3rd) ring layer of white on the outside — since we have 6px padding in between icon and text, this would mean the text will be flush against that last white ring layer.
For option 1, the outermost outline would overlap the text unless we change the margin of the dismiss button.
I also think this technically might not meet the WCAG SC 2.4.13 Focus Appearance:
has a contrast ratio of at least 3:1 between the same pixels in the focused and unfocused states.
Since these outlines would be placed on top of the tag element, they may not always contrast 3:1 with the pixels they're replacing (at least with the base selected tag and possibly with others).
- Add additional padding around the Clear Icon when in focus to push the rings to the 'outside' of the tag element. This would allow us to have 3 edges that touch the white background color and maintain our 3:1 ratio on 3 sides. Only the inner left border wouldn't meet that ratio in certain cases but I think that's ok.
Option 2 would cause fewer problems IMO. If we go with option two, we'd need to change the size of the dismiss button and compensate for that change with negative margins to maintain the same layout.
This gets the outlines outside the tag, but this may cause hover to look kinda weird in some cases:
Outside of that minor issue, I think this solution would work fine.
Since it seems like what we have currently in this PR almost meets our contrast target, there's another option we could consider. We could use a slightly darker theme-secondary
color for the outline on dismiss buttons within selected tags. This would allow us to maintain the current button size, not have to adjust the layout, and hit our 3:1 target contrast ratio.
The lowest effective contrast ratio I could find is between the blue ring and the base selected tag background, which is a 1.87:1 ratio (All other color combinations exceed the 3:1 ratio). If we use the theme-secondary-500
instead of theme-secondary-400
for selected tags dismiss button, we should exceed the 3:1 ratio in all cases.
theme-secondary-500
outlineAll high contrast versions would also meet or exceed the 3:1 ratio.
@CGuindon let me know what you think and I'll implement whatever you think the best option is.
The only observation I have is to clarify if a use case where the tag is an anchor and it also contains a dismiss button is a valid one?
<a class="s-tag" href="#">javascript <button class="s-tag--dismiss">@Svg.ClearSm</button></a>
if no, we should probably mention in the
s-tag--dismiss
documentation that the dismiss action can only be used in non focusable tags.If it's valid, we should add an example.
@giamir From W3C HTML reference:
… there must be no interactive content descendant,
a
element descendant, or descendant with thetabindex
attribute specified.
It's invalid to have nested focusable elements (so a button
cannot be nested in an a
tag). I've updated the examples to show this and I've updated the description for .s-tag--dismiss
to describe the appropriate markup.
@dancormier Alternate option gives me pause because of theming... and in some cases we don't have the 3:1 ratio on both sides so I think Option 2 is safest and doesn't feel that bad for the hover state weirdness.
Option 2
I've updated the dismiss button so the theme-secondary outline is shown outside of the tag element as shown in option two here. In order to do this, I needed to shift the location of the dismiss button horizontally slightly (I believe it winds up being 1px to the right).
@CGuindon @giamir Can you two give this a rereview when you get a moment and let me know if this seems ok?
Addresses STACKS-541. See also #1612, #1613 for related PRs.
This commit also adds some minor styling updates to account for button-based
.s-tag--dismiss
elements.These changes should result in no changes to existing
.s-tag--dismiss
elements, but the docs now display the semantically proper usage of a button element for theX
button with the parent.s-tag
being rendered as a span and consumers can now use a button element for the.s-tag--dismiss
child without needing to apply reset classes.