fix(ActionRow): align DOM and visual order in stacked mode#4271
fix(ActionRow): align DOM and visual order in stacked mode#4271e0d wants to merge 2 commits intorelease-23.xfrom
Conversation
✅ Deploy Preview for paragon-openedx-v23 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79c695fcb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| flex-grow: 1; | ||
| align-items: center; | ||
| flex-direction: column-reverse; | ||
| flex-direction: column; |
There was a problem hiding this comment.
Fix stacked item spacing after switching to column flow
Changing pgn__action-row-stacked to flex-direction: column without updating the sibling spacing rule causes the vertical gap to be applied below later items (margin-bottom) instead of between adjacent actions. In stacked mode this makes controls visually bunch together (no inter-button gap) while adding extra trailing space at the bottom, so the layout regresses for any multi-action stack.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex Does the change at line 28 of this file resolve this issue satisfactorily?
In isStacked mode, flex-direction: column-reverse placed the primary action (last DOM child) at the visual top but keyboard focus still reached it last, violating WCAG 1.3.2 Meaningful Sequence and 2.4.3 Focus Order. Fix (Option A): reverse children in DOM order when isStacked is true and change the SCSS to flex-direction: column. The primary action is now first in both DOM and visual order — a sighted keyboard user tabs to it first, matching what they see. Consumers continue passing the primary action as the last child (consistent with horizontal mode); the component handles reversal internally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
79c695f to
5c2a355
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-23.x #4271 +/- ##
=============================================
Coverage 94.40% 94.41%
=============================================
Files 242 242
Lines 4309 4312 +3
Branches 1020 984 -36
=============================================
+ Hits 4068 4071 +3
Misses 237 237
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@kblemel in the WG meeting you mentioned finding documentation/discussions about |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Overall I think this makes sense.
I did notice one edge case when using the following example code to simulate an application that switches between stacked and non-stacked (I'm thinking this would likely be done for responsive design with useMediaQuery)
Code used on docs site to test
() => {
const [stackIt, setStackIt] = React.useState(false);
React.useEffect(() => {
const interval = setInterval(() => {
setStackIt(prev => !prev);
}, 2000); // toggles every 2 seconds
return () => clearInterval(interval);
}, []);
return (
<ActionRow isStacked={stackIt}>
<p className="x-small">
Bespoke leggings yuccie, portland umami readymade craft beer vaporware sriracha.
</p>
<Button variant="tertiary">
Go back
</Button>
<Button variant="primary">
Continue
</Button>
</ActionRow>
);
}Behavior before PR
The element selected by the keyboard stays selected when the component switches between stacked and non-stacked.
Screencast.From.2026-05-07.10-24-52.mp4
Behavior after PR
The element selected by the keyboard is no longer selected when the component switches between stacked and non-stacked
Screencast.From.2026-05-07.10-26-59.mp4
@kblemel - any thoughts on that change? Also it'd be great to know if any of the conversations/documentation you found mentioned something like that (I assume the plugin you mentioned does something similar to what this PR is doing and would result in similar behavior)
| @@ -0,0 +1,98 @@ | |||
| # CLAUDE.md | |||
There was a problem hiding this comment.
I know we've discussed adding CLAUDE.md and AGENTS.md files to repos in the org before, but that should definitely happen as a separate PR.
| @@ -0,0 +1,394 @@ | |||
| # WCAG 2.2 AA Accessibility Audit — Paragon Component Library | |||
There was a problem hiding this comment.
Very cool to see this file for context, but it shouldn't be part of this PR
|
|
||
| # Local Netlify folder | ||
| .netlify | ||
| .claude/ |
There was a problem hiding this comment.
Not sure about "claude" as the section comment here but I figure making it clear this isn't part of the "Local Netlify folder" would be good
| .claude/ | |
| # claude | |
| .claude/ |
There was a problem hiding this comment.
What's your recommendation? I don't have a strong commitment to this change.
| @@ -0,0 +1,154 @@ | |||
| # WCAG 2.2 AA Accessibility Audit — ActionRow | |||
There was a problem hiding this comment.
Very cool to see this file for context, but it shouldn't be part of this PR
Adds WCAG 2.2 AA audit documentation for ActionRow, top-level ACCESSIBILITY_AUDIT.md overview, ActionRow component PDF reference, and CLAUDE.md project instructions. Excludes .claude/ from tracking. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
d8d8fd7 to
4e3c7bd
Compare
|
I have vague memory of this being a design choice with external input -- someone wanted Next to be earlier than Back because Back is much less likely to be needed, so it's a usability improvement. This is the kind of thing I don't have a strong opinion on, so defer to whatever Design wants. |
Maybe that's true, but I'd argue the visual order and tab order should still be the same in that case. Do you disagree? |
In isStacked mode, flex-direction: column-reverse placed the primary action (last DOM child) at the visual top but keyboard focus still reached it last, violating WCAG 1.3.2 Meaningful Sequence and 2.4.3 Focus Order.
Fix (Option A): reverse children in DOM order when isStacked is true and change the SCSS to flex-direction: column. The primary action is now first in both DOM and visual order — a sighted keyboard user tabs to it first, matching what they see.
Consumers continue passing the primary action as the last child (consistent with horizontal mode); the component handles reversal internally.
Description
Include a description of your changes here, along with a link to any relevant Jira tickets and/or Github issues.
Deploy Preview
Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).
Merge Checklist
exampleapp?Post-merge Checklist