feat(sidebar): align with fusion ds - #8145
Conversation
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Modernizes Sidebar to align with Fusion design, responsive behavior, and accessibility conventions.
Changes:
- Adds fluid desktop and full-width mobile layouts using Fusion tokens.
- Adds semantic header labeling, variants, gradient keyline, and deprecations.
- Updates stories, documentation, and browser/unit coverage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/components/sidebar/sidebar.component.tsx |
Updates API, accessibility, and modal integration. |
src/components/sidebar/sidebar.style.ts |
Implements responsive and tokenized styling. |
src/components/sidebar/sidebar.test.tsx |
Covers semantics, variants, and styles. |
src/components/sidebar/sidebar.pw.tsx |
Tests responsive browser behavior. |
src/components/sidebar/components.test-pw.tsx |
Updates Playwright fixtures. |
src/components/sidebar/sidebar.stories.tsx |
Adds responsive and visual examples. |
src/components/sidebar/sidebar-test.stories.tsx |
Updates visual test stories. |
src/components/sidebar/sidebar.mdx |
Documents the updated API and behavior. |
src/components/sidebar/__internal__/sidebar-header/sidebar-header.component.tsx |
Adds semantic titles and keyline rendering. |
src/components/sidebar/__internal__/sidebar-header/sidebar-header.style.ts |
Applies header variants and Fusion tokens. |
skills/carbon-react/components/sidebar.md |
Excluded from review by policy. |
Files excluded by content exclusion policy (1)
- skills/carbon-react/components/sidebar.md
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4f9e8fa to
1c93c90
Compare
1c93c90 to
0096cd0
Compare
| data-role="sidebar-header" | ||
| p="27px 32px 32px" | ||
| headerVariant={headerVariant} | ||
| p="var(--global-space-comp-xl)" |
There was a problem hiding this comment.
suggestion, instead of passing padding down like this, we can just apply it directly in the css of the styled component.
You can do this here in StyledSidebarHeader and SidebarSubHeader, once these have been replaced you can also remove the padding import from styled-system in sidebar-header.style.ts
There was a problem hiding this comment.
If I remove that import as suggested, public headerPadding and subheaderPadding props stop doing anything.
But good catch on the approach 'p=...', I will move default padding to css, so this is handled in one place.
| const inverse = $headerVariant === "inverse" || $headerVariant === "dark"; | ||
|
|
||
| return css` | ||
| background-color: ${inverse |
There was a problem hiding this comment.
nitpick: you could do the ternary inside a template literal like so
var(--container-standard-${inverse ? "inverse" : ""}-bg-default)
or something alone those lines to save a little bit of lines here, but what you have is fine and completely acceptable
There was a problem hiding this comment.
Suggestion as literally written looks broken) Consider inverse = false => var(--container-standard**--**bg-default). The hyphens catch an eye and I think we accidentally can invite a morse code into our CSS)
If you insist on change, I can introduce const variant suffix = inverse ? "-inverse" : "". Do you think it worth it?
| display: flex; | ||
| justify-content: space-between; | ||
| gap: var(--spacing200); | ||
| gap: var(--global-space-comp-l); |
There was a problem hiding this comment.
question: should this gap be the same for all sizes, or should we not be using the component size to dictate this gap?
There was a problem hiding this comment.
Same comment applies to other fixed size usages in the PR 👍
There was a problem hiding this comment.
Let me double-check
| color: var(--container-standard-inverse-txt-default); | ||
| } | ||
|
|
||
| &:hover ${StyledIcon}, &:active ${StyledIcon}, &:focus ${StyledIcon} { |
There was a problem hiding this comment.
suggestion (non-blocking): you may be able to group these together like:
&:is(:hover, :active, :focus) ${StyledIcon} { color: ...; }
There was a problem hiding this comment.
Nice simplification, will apply it
|
|
||
| ### Responsive behavior | ||
|
|
||
| At 768px and below, the Sidebar fills the viewport and scrolls its content regions together. Above 768px, it uses its desktop width and keeps a sticky footer visible while the content scrolls. |
There was a problem hiding this comment.
suggestion: I think some of the content here is repeated as the same responsive behaviour is also mentioned above on line 33
| subHeader, | ||
| position = "right", | ||
| size = "medium", | ||
| size, |
There was a problem hiding this comment.
question: is there any reason why "medium" is no longer the default here?
There was a problem hiding this comment.
Size prop is now deprecated
| }; | ||
|
|
||
| const closeButton = renderCloseButton(); | ||
| const hasHeader = Boolean(header); |
There was a problem hiding this comment.
question: do we need these consts here? just checking the props presence seemed to be doing the job
There was a problem hiding this comment.
They are justifed as used more than once, and make usage conditions in difference places clearer
|
|
||
| test.describe("Focus management and interaction tests for Sidebar component", () => { | ||
| test("should render component with focusableContainers", async ({ | ||
| test("allows an additional focusable container to remain interactive", async ({ |
There was a problem hiding this comment.
question: more of a general question on the testing approach here, are there any tests below which you think may be better suited as static chromatic snapshots or interaction stories which trigger a chromatic snapshot?
There was a problem hiding this comment.
Agree, visual layout should be covered by Chromatic, which is why ResponsiveBehaviour has snapshots on both sides of the breakpoints. But why playwright here - because these tests verify focus order, scrolling, modal-interactions - visual snapshot cannot prove that.
Good split to think about. Probably interaction stories can simulate tab/click sequence and even assert focus, for example, but they show nothing about which element has focus. I mean a human reviewing snapshot has to visually confirm, 'yes, that focus ring is on that button, and not not on the other button nearby'. Also, the other real constraint is sequencing, even not visibility. Consider test case, like 'when is opened, then closed, then ..., then check focus again', - it means capturing several focus movements in one flow, and this not always the same is capturing one state per story.
Still I prefer to align with team/project approach. Ping me if this case need to be reworked!
BREAKING CHANGE: Sidebars without an explicit size or width now use a fluid 30vw width with a 288px minimum instead of the 514px medium width. At 768px and below, Sidebars fill the viewport and hide the modal dimmer
33eb67c to
22c07e5
Compare
Proposed behaviour
Updated Sidebar follows the latest responsive and visual design:
30vwdesktop width with a288pxminimum.768pxand below.768pxand below.h1elements and automatically associate string and custom-node headers with the dialog.typicalandinverseheader variants while retaininglightanddarkas deprecated aliases.gradientKeyLineheader treatment.positionand presetsizeprops while retaining explicit legacy values.BREAKING CHANGE: Sidebars without an explicit size or width now use a fluid 30vw width with a 288px minimum instead of the 514px medium width. At 768px and below, Sidebars fill the viewport and hide the modal dimmer.
Current behaviour
Sidebar currently:
mediumwidth of 514px.lightanddarkheader variants.Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions
30vw, with a 288px minimum.size=“extra-large”and oversized custom widths are capped at the viewport width.h1and supplies the dialog’s accessible name.aria-labelandaria-labelledbyvalues still override the generated association.headerVariant=“typical”andheaderVariant=“inverse”.lightanddarkvalues retain their equivalent treatments.gradientKeyLine.