[Button] Fix Next/Previous buttons staying in clicked state - #7941
[Button] Fix Next/Previous buttons staying in clicked state#7941vermarjun wants to merge 2 commits into
Conversation
The base ButtonStyle rule paints both :hover and :focus with the dark activeColor (#3c494f), but the $primary, $secondary and $outlined variant blocks only re-declare :hover. A mouse click leaves the button focused, so the base :focus rule wins and the button stays dark until the user clicks somewhere else. Scope that base rule to :focus-visible. A mouse click no longer triggers it, and keyboard focus keeps its visible indicator. Fixes layer5io#7940 Signed-off-by: vermarjun <vermarjun26@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe button focus selector changed from ChangesButton focus behavior
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized styling change restores the intended button colors after pointer clicks while preserving keyboard focus indication; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@leecalcote could you take a look when you get a chance? One line in Two calls I'd like your read on:
|
Sure, @vermarjun. I'm taking a look... |
leecalcote
left a comment
There was a problem hiding this comment.
Thanks!
The $primary, $secondary, $outlined blocks only override :hover, not :focus, yeah?
After this change a keyboard-focused variant button will still show the base activeColor rather than the variant's own color. How does this manifest in other button variants? Do we potentially need to add &:focus-visible to each variant block?
Prettier skipped? If --no-verify was used, let's get a follow up commit.
The file was not prettier formatted on master, so the lint-staged hook rewrites it wholesale. Landing that as its own commit keeps the :focus-visible fix in e4c77a5 reviewable at one line. No behavior change. Signed-off-by: vermarjun <vermarjun26@gmail.com>
|
@leecalcote yes, that's right. On whether they need their own
Two things worth pulling apart here. The dark-on-keyboard-focus behavior isn't something this PR introduces. On master If you'd rather variants kept their own color while focused, I'd write it as an outline instead of a background swap: &:focus-visible {
outline: 3px solid ${props => props.theme.secondaryColor};
outline-offset: 2px;
}Every variant keeps its identity color, and a ring is easier to get right on contrast than recoloring three buttons. But it sets the focus language for the whole site, not just this file. Prettier is done, pushed as cff8761 with the hook running this time. One thing I ran into while doing it. |
|
Preview deployment for PR #7941 removed. This PR preview was automatically pruned because we keep only the 3 most recently updated previews on GitHub Pages to stay within deployment size limits. If needed, push a new commit to this PR to generate a fresh preview. |
Description
This PR fixes #7940
Notes for Reviewers
The stuck state comes from
reusecore/Button, not from the pagination component.The base
ButtonStylerule paints:hoverand:focuswith the same declaration:The three variant blocks below it all re-declare
:hover. None of them re-declare:focus. A mouse click leaves the button focused, so the base:focusrule keeps winning and the button sits at#3C494Funtil focus moves. Clicking anywhere else blurs it, which is the "only comes back after clicking somewhere else" part of the report.The change scopes that base rule to
:focus-visible. Browsers don't match:focus-visiblewhen a button is clicked with a pointer, so the button drops straight back to its own color. They do match it on tab, so keyboard users keep the same focus indicator they had before.Measured on
/resourceswithgatsby develop, reading computedbackground-colorafter a real click with the pointer moved off the button:$primary#EBC017#3C494F#EBC017$secondary#00B39F#3C494F#00B39FTab focus still resolves to
#3C494Fon both, so nothing is lost for keyboard navigation.Before, after clicking Next:
After:
Two notes:
$primary,$secondaryand$outlinedbutton on the site held the dark state after a click.btn.style.jsisn't prettier formatted onmaster, so thelint-stagedpre-commit hook rewrites the whole file (75 insertions, 71 deletions) alongside the fix. I committed with--no-verifyto keep this reviewable at one line. Happy to push the reformat as a separate commit if you want it.Signed commits
Summary by CodeRabbit