CORE-2008: Migrate feature components (MessageBox, Banner, Tree, ToggleButtonGroup) - #146
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Ref-forwarding regressions and Banner close-button visual and accessibility regressions must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Migrates MessageBox, Banner, Tree, and ToggleButtonGroup from styled-components to token-backed plain CSS.
Changes:
- Replaces styled wrappers with class-composing React components.
- Adds component stylesheets and theme-token validation.
- Updates tests, snapshots, stories, and migration documentation.
File summaries
| File | Description |
|---|---|
src/theme/tokens.spec.ts |
Allows preserved Banner colors. |
src/components/Tree/Tree.tsx |
Replaces styled Tree wrappers; ref forwarding must be restored. |
src/components/Tree/Tree.spec.tsx |
Tests classes and composition. |
src/components/Tree/Tree.css |
Adds Tree styles. |
src/components/Tree/__snapshots__/Tree.spec.tsx.snap |
Updates Tree snapshot. |
src/components/ToggleButtonGroup/ToggleButtonGroup.css |
Adds toggle styles. |
src/components/ToggleButtonGroup/styles.tsx |
Adds class-based wrappers; ref forwarding must be restored. |
src/components/ToggleButtonGroup/styles.ts |
Removes styled-components implementation. |
src/components/ToggleButtonGroup.spec.tsx |
Tests class composition. |
src/components/MessageBox/MessageBox.tsx |
Replaces styled elements; ref forwarding must be restored. |
src/components/MessageBox/MessageBox.spec.tsx |
Tests margin-variable binding. |
src/components/MessageBox/MessageBox.css |
Adds MessageBox styles. |
src/components/MessageBox/__snapshots__/MessageBox.spec.tsx.snap |
Updates MessageBox snapshots. |
src/components/Banner/Banner.tsx |
Replaces styled elements; ref forwarding must be restored. |
src/components/Banner/Banner.stories.tsx |
Removes styled story container. |
src/components/Banner/Banner.spec.tsx |
Tests severity classes and props. |
src/components/Banner/Banner.css |
Adds severity and close-button styles; missing inherited layout, disabled, and focus styles must be restored. |
src/components/Banner/__snapshots__/Banner.spec.tsx.snap |
Updates Banner snapshots. |
src/components/__snapshots__/ToggleButtonGroup.spec.tsx.snap |
Updates toggle snapshots. |
src/components/__snapshots__/ErrorModal.spec.tsx.snap |
Updates MessageBox-derived classes. |
src/components/__snapshots__/Error.spec.tsx.snap |
Updates error-component classes. |
CHANGELOG.md |
Documents migration and breaking changes. |
Review details
Suppressed comments (3)
src/components/MessageBox/MessageBox.tsx:33
- The old
styled.h3export forwarded refs; this ordinary function component no longer does under React 17. Wrap it inReact.forwardRef<HTMLHeadingElement, BoxProps<'h3'>>and pass the ref to the heading.
export const BoxHeading = ({ className, ...props }: BoxProps<'h3'>) => (
<h3 {...props} className={classNames('message-box-heading', className)} />
);
src/components/MessageBox/MessageBox.tsx:37
- The old
styled.divexport forwarded refs; this ordinary function component no longer does under React 17. Wrap it withforwardRefso existing consumers can still access the body element.
export const BoxBody = ({ className, ...props }: BoxProps<'div'>) => (
<div {...props} className={classNames('message-box-body', className)} />
);
src/components/MessageBox/MessageBox.tsx:41
- The old
styled.divexport forwarded refs; this ordinary function component no longer does under React 17. Wrap it withforwardRefso the event-id element remains accessible through the public ref API.
export const BoxEventId = ({ className, ...props }: BoxProps<'div'>) => (
<div {...props} className={classNames('message-box-event-id', className)} />
);
- Files reviewed: 22/22 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .banner-close-button { | ||
| color: var(--banner-color); | ||
| overflow: visible; | ||
| background: none; | ||
| border: none; | ||
| padding: 0; | ||
| font: inherit; | ||
| cursor: pointer; | ||
| outline: inherit; | ||
| box-shadow: none; | ||
| margin-left: 2.4rem; | ||
| } |
| outline: inherit; | ||
| box-shadow: none; | ||
| margin-left: 2.4rem; | ||
| } |
| export const Severity = ({ className, ...props }: WithVariableStyle<'span'>) => ( | ||
| <span {...props} className={classNames('banner-severity', className)} /> | ||
| ); |
| `; | ||
| } | ||
|
|
||
| export const BoxWrapper = ({ margin, className, style, ...props }: BoxWrapperProps) => ( |
| export const StyledToggleButtonGroup = ({ className, ...props }: ToggleButtonGroupProps) => ( | ||
| <ToggleButtonGroup | ||
| {...props} | ||
| className={composeRenderProps(className, (resolved) => classNames('toggle-button-group', resolved))} | ||
| /> |
| export const Tree = <T extends object>({ className, ...props }: RAC.TreeProps<T>) => ( | ||
| <RAC.Tree | ||
| {...props} | ||
| className={RAC.composeRenderProps(className, (resolved) => classNames('tree', resolved))} | ||
| /> |
…plain CSS Replaces the styled-components definitions in these four components with sibling stylesheets bound to the --ox-* theme tokens, following the pattern established by the Button, Modal and composite-component migrations. The exported styled pieces (BoxWrapper, BoxHeading, BoxBody, BoxEventId, StyledBanner, Severity, CloseButton, Tree, TreeItem, TreeChevron, StyledToggleButtonGroup, StyledToggleButton) keep their names and props; the react-aria wrappers compose className through composeRenderProps so a caller's render-callback form is not dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Snapshots record the shape; these assert the parts that carry meaning — the banner tone class landing on both the banner and its close button, the margin binding on MessageBox, the tree chevron structure, and className composition on the react-aria wrappers that consumers wrap with styled(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
effcf24 to
93b0987
Compare
|
Rebased — history rewritten, base unchanged. To pick it up: Roy asked for #143 to be split for review, so its branch was rewritten and this PR had to move with it. The stack is now This one stays on #143 rather than moving to #150 like #140 and #141 did, because it genuinely overlaps the sweep: This branch was carrying ten stale commits. It forked from an older state of the CORE-2720 branch and was never rebased through the fifth- and sixth-round rewrites, so it held its own copies of #137 and the whole CORE-2720 series — including duplicates of Roy's three commits. Those are gone; what is replayed is the two commits that are actually this PR's work. 12 commits → 2, 496 tests / 41 suites / 114 snapshots pass; typecheck clean, no conflicts, no snapshot regeneration. Worth a look at the diff before picking this back up — it should now show only MessageBox, Banner, Tree, ToggleButtonGroup and the CHANGELOG, where before it showed the token work as well. 🤖 Generated with Claude Code |
Migrates the last four feature components off styled-components and onto plain CSS bound to the
--ox-*theme tokens, following the pattern set by the Button, Modal and composite-component migrations.Jira: https://openstax.atlassian.net/browse/CORE-2008 — the full implementation plan is in the ticket description.
Based on
CORE-2720-global-css-theme-tokens, notmain, as requested on the ticket. Review the diff against that branch.What changed
MessageBox.css.message-box,.message-box-heading,.message-box-body,.message-box-event-idBanner.css.banner,.banner-note/.banner-warning/.banner-error,.banner-severity,.banner-close-buttonTree.css.tree,.tree-item,.tree-chevronToggleButtonGroup.css.toggle-button-group,.toggle-buttonTree/TreeCheckbox.tsxis listed on the ticket but was already migrated with the Checkbox work and hardened by CORE-2708/CORE-2710. It is verified here, not re-migrated.Decisions worth a reviewer's attention
.banner-note/.banner-warning/.banner-errorset--banner-bg,--banner-colorand--banner-border-color. The tone class goes on both the banner and its close button, soCloseButtonkeeps its severity colour when a consumer renders it outside aStyledBanner— which assignments does not do today, but the oldstyled(Button)supported.CloseButtonis now a self-contained<button class="banner-close-button">rather than a wrappedButton. The original wrappedButtonand then unset every styleButtongave it, and only won because styled-components injects its sheet last. In plain CSS the two selectors have equal specificity, so the result would depend on module evaluation order. A self-contained class removes the cascade fight entirely.#fff5e0,#976502,#fdbd3eand#f8e8ea. Two are near-misses for palette entries (tangerine #ffbd3e,paleRed #fbe7ea), so snapping them would be a visual change rather than a refactor. They go inKNOWN_OFF_PALETTEwith a reason each. Happy to promote them topalette.tsas banner design tokens instead if you'd rather — that felt like a design call, not a migration one..tree[data-expanded] .react-aria-Button[slot=chevron] svgcannot match:TreeChevronalways supplies a className to itsRAC.Button, so the button never carries RAC's defaultreact-aria-Buttonclass. The rotation you actually see comes from.tree-item[data-expanded] svg. Kept verbatim with a comment, because deleting it would be a behaviour change rather than a refactor. Say the word and I'll drop it.TreeChevron's button is always given a class (tree-chevron-button) for the same reason — under styled-components it always had one, and letting RAC fall back to its default class would make the rule above suddenly live.CloseButtonno longer leaksseverityto the DOM.styled(Button)forwarded unknown props through to a custom component, so the rendered<button>carriedseverity="warning". It doesn't any more; visible in the snapshot diff.typeis deliberately left unset onCloseButton. The original inherited<button>'ssubmitdefault. Defaulting a dismiss button tosubmitinside a form is a latent bug, but fixing it is a behaviour change and belongs in its own ticket.Breaking change for openstax/assessments
BoxWrapperis no longer a styled-component, so it can't be used as a styled-components component selector.assessmentsdoes exactly that inpackages/frontend/src/assessments/screens/Preview/styled.tsx:That needs to become a
.message-box { ... }selector. It is a separate PR in assessments, needed when it next bumps its ui-components tag — this PR does not touch assessments.Everything else is source-compatible. I checked every use of these exports in openstax/assignments and openstax/assessments:
StyledBanner,CloseButton,Tree,TreeItem,TreeItemContent,TreeChevron,MessageBoxandToggleButtonGroupkeep their props and rendered structure. In particular assignments wraps the group withstyled(UI.ToggleButtonGroup)in two places, so the wrapper composes the caller'sclassNamerather than replacing it.Testing
sc-bczRLJ hWiIYF).src/theme/tokens.spec.tsis the real guard: it fails on any literal in the new stylesheets that duplicates a theme value, on any new off-palette colour, and on any--ox-*token that doesn't exist.🤖 Generated with Claude Code