feat: Invert button group icons for better contrast #352
Conversation
|
@abovedave How to I test stuff in the editor repo? I noticed I can't add the "PR environment" label? |
✅ Deploy Preview for stacks-editor ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@CGuindon added the deploy preview, I think it didn't do automatically as the PR is from my personal account. |
|
The focus state looks worse with the reverse colors. For HC mode and AAA we're supposed to have 2 px in between colors that need the contrast ratio. This would be a small, low severity issue since we still have at least 1 px line in there (and it's for AAA not AA, which we only support for HC mode) but if there's a better way to do the fill or we need to increase to Focus on current Focus with py4 |
|
@CGuindon good catch. We could remove the inset on the focus? 
|
|
@abovedave I'd be ok with that for this type of icon only button group edge case. Would still love a gut check from @dancormier though. |
|
@abovedave Can't see the changes in the PR — you might need to kick or something to regenerate a PR-env |
|
@CGuindon try now! |
This reverts commit 8c26b03. # Conflicts: # package-lock.json # package.json
… into STACKS-555
|
Before merging we should rollback the Also are we ok to add these bespoke styles here? Should we solve this at Stacks Classic level? We try to keep bespoke editor styles to a minimum in general. |
|
@giamir reverted package changes. They were from some high vulnerability errors on install, but I assume they are the other PR's from dependabot. If I can get access to this repo I can deal with them if you like? |
@abovedave I gave you write access to the repo. Feel free to take care of the violations if you have time in a separate PR or commit. Thank you. |
I wanted to wait for Dan's opinion before merging. I personally do think it is something we should solve and document in Stacks Classic: https://github.com/StackExchange/Stacks/blob/develop/lib/components/button-group/button-group.less @abovedave what are your thoughts? If there is urgency to fix this we can merge this PR and add a follow up ticket to move the styles back in Stacks classic if we agree. If there is no urgency maybe we should directly open a PR in Stacks Classic instead. |
|
The complication is this is a bit of a mash-up component. We're using the radio inputs with the Another fix might be to refactor this to be more of a pure button group (no radio inputs) and use Stack's atomic classes to achieve the desired visuals. It looks like the outer I think on balance a few lines of CSS here for the high-is priority accessibility fix are probably worth the trade-off vs unpicking the above! |
There was a problem hiding this comment.
@abovedave I am ok to merge this then, thanks for the explanation. I only have a small ask to ensure the rules are scoped only to the editor. I have added a comment inline.
After that, we can merge. Thanks.
|
@abovedave Let me know if you need assistance to cut a patch release: |
Closes internal ticket [STACKS-555].
Currently the button groups in Stacks make the text bold on selected state. However the editor uses icons in place of text, which we obiously can't easily 'bolden'.
This PR adds some override CSS to button groups for the editor to invert the icon against a darker background, making it much more accessible.