fix: addressing misbehaving margin props for Banner#266
fix: addressing misbehaving margin props for Banner#266haoruikun-cb wants to merge 4 commits intomasterfrom
Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
haoruikun-cb
had a problem deploying
to
production
GitHub Actions
Error
haoruikun-cb
temporarily deployed
to
production
GitHub Actions
Inactive
haoruikun-cb
force-pushed
the
harry/banner-margin-exp
branch
from
afa03c3 to
d1a161f
Compare
haoruikun-cb
had a problem deploying
to
production
GitHub Actions
Error
haoruikun-cb
changed the title
haoruikun-cb
temporarily deployed
to
production
GitHub Actions
Inactive
|
|
||
| return hasResponsiveOverrides ? widthByBreakpoint : baseWidth; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Debate question: is it worth introducing complexity like this for addressing this specific use case?
| // Prefer explicit start/end over marginX (marginX sets both sides). | ||
| const startSpaceToken = | ||
| normalizeNegativeSpace(effectiveMarginStart) ?? normalizeNegativeSpace(effectiveMarginX); | ||
| const endSpaceToken = | ||
| normalizeNegativeSpace(effectiveMarginEnd) ?? normalizeNegativeSpace(effectiveMarginX); |
There was a problem hiding this comment.
Do other components prioritize marginStart / marginEnd styling over marginX? We should keep the styling API consistent with other components since this is a common styling prop
| return str.slice(1); | ||
| }; | ||
|
|
||
| const spaceVar = (spaceToken: string) => `var(--space-${spaceToken.replace('.', '_')})`; |
There was a problem hiding this comment.
This might be better utilized inside the component and with the useTheme hook. We could avoid these normalization methods and just append 'px' to the theme token value
| <Box | ||
| position="relative" | ||
| width="100%" | ||
| width={showDismiss ? '100%' : wrapperWidth} |
There was a problem hiding this comment.
If using width="100%" is the root cause for these styling issues, could it be removed? There could be another CSS solution which has the Banner's width grow without using flex. The PR you linked that introduced this regression was addressing an issue with the Banner's height. It would be better to address the root cause of this issue. This solution does seem a bit over-engineered.
Could something like this work?
| width={showDismiss ? '100%' : wrapperWidth} | |
| // Replace with margin props based on order priority | |
| style={{ width: `calc(100% + ${marginX}px` }} |
| desktop?: string; | ||
| } = { base: baseWidth }; | ||
|
|
||
| const computeForBreakpoint = (breakpoint: 'phone' | 'tablet' | 'desktop') => { |
There was a problem hiding this comment.
Let's use the DeviceBreakpoint type from src/styles/media.ts
| const computeForBreakpoint = (breakpoint: 'phone' | 'tablet' | 'desktop') => { | |
| const computeForBreakpoint = (breakpoint: Pick<DeviceBreakpoint, 'phone' | 'tablet' | 'desktop') => { |
3 participants
What changed? Why?
Summary
In a previous PR (#209), we fixed a web
Bannerlayout regression by moving fromflexGrow-based sizing to explicitwidth/height. That change was necessary to resolve a production issue and also cleaned up a few edge-case layout glitches (see the original PR for details).However, after the change, the
margin*style props started behaving unexpectedly. CDSmargin*props are implemented as negative margins to create a “bleed” effect. When the wrapper is sized withwidth="100%", a negative horizontal margin simply shifts the Banner left/right while keeping its width at 100%, which removes the intended bleed. The visual result is that the Banner appears misaligned or clipped instead of extending past the container.To restore the expected behavior, we now compute and apply an explicit wrapper width that accounts for negative margins (e.g.
calc(100% + <startBleed> + <endBleed>)). This makes the Banner span the full container and bleed past it by the negative margin amount, and it correctly tracks responsive margin overrides at each breakpoint.Root cause (required for bugfixes)
The earlier fix switched the Banner wrapper from
flexGrowsizing to explicitwidth, but ourmargin*props are implemented as negative margins. With a fixed width="100%", negative horizontal margins only shift the element instead of expanding its visual footprint, so the intended “bleed” past the container was lost. This made margin-based layouts look misaligned or clipped.UI changes
Web Old
544022633-88751421-3d52-4fdd-9aac-393a1fc0c204.mov
Web New
Screen.Recording.2026-02-02.at.2.50.50.PM.mov
Screen.Recording.2026-02-02.at.2.57.03.PM.mov
Testing
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false