Harryhao/cds 1513 consolidate listcell border props#352
Conversation
haoruikun-cb
force-pushed
the
harryhao/cds-1513-consolidate-listcell-border-props
branch
from
55f3eb7 to
f84b95d
Compare
cb-ekuersch
left a comment
cb-ekuersch
left a comment
There was a problem hiding this comment.
please yarn install and verify your unit tests, lint and typecheck commands pass
| > | ||
| Spacing variant: {spacingVariant} | ||
| </Switch> | ||
| <ListCell |
There was a problem hiding this comment.
nitL are this many examples necessary? Try to limit stories to a few key examples. Keep in mind the primary use case is visual regression testing and visual documentation/interaction.
There was a problem hiding this comment.
Ok that make sense, I'll cut down the story here and only leave interesting/important examples
|
|
||
| const { marginX: innerSpacingMarginX, ...innerSpacingWithoutMarginX } = innerSpacing; | ||
|
|
||
| const borderProps = useMemo( |
There was a problem hiding this comment.
please leave a comment:
- explaining why we need to collect the border props
- a link to the linear issue we created to remove the outer Cell element (i believe we have one, if not lets create a breaking change cds-v10 issue for it)
| interactablePressedBorderColor, | ||
| interactablePressedOpacity, | ||
| } from './interactableCSSProperties'; | ||
| import { MediaQueryContext } from './MediaQueryProvider'; |
There was a problem hiding this comment.
this is technically a breaking change. Before Interactible didn't have to be used inside the MediaQueryProvider (even tho it probably is in most cases already)
There was a problem hiding this comment.
I think this should be an OK breaking change to make since Pressable are being used inside MediaQueryProvider as well so customer technically don't need to change anything in most cases. But we need to make this thing clear in our v9 documentation so customer is aware of this.
| } | ||
| `; | ||
|
|
||
| const isResponsiveValue = <T,>(value: ResponsiveProp<T>): value is ResponsiveValue<T> => |
There was a problem hiding this comment.
nit: lets put this in a web util. Maybe if we already have a responsive module or something similar it can live there
| /** Set element to block and expand to 100% width. */ | ||
| block?: boolean; | ||
| /** Border color of the element. */ | ||
| borderColor?: ThemeVars.Color; |
There was a problem hiding this comment.
because this comes from Box right?
There was a problem hiding this comment.
Yes that's correct.
| const Component = (as ?? interactableDefaultElement) satisfies React.ElementType; | ||
| const theme = useTheme(); | ||
| const mediaQueryContext = useContext(MediaQueryContext); | ||
| const resolvedBorderColor = useMemo( |
There was a problem hiding this comment.
why do we need to do this? I thought responsive props resolve for us automatically?
There was a problem hiding this comment.
If the borderColor is being applied to Box directly, we shouldn't need to write this logic. However it's being applied to a style prop. It has a dedicated logic to compute what style is being applied to the Box based on the colors passed in:
There are some typecheck and lint issues coming from cds-v9 branch. I can draft a separate PR to fix that |
packages/web/src/utils/responsive.ts
Outdated
| * @returns The resolved value for the current breakpoint, or the fallback when | ||
| * getSnapshot is not provided | ||
| */ | ||
| export const resolveResponsiveProp = <T>( |
There was a problem hiding this comment.
@haoruikun-cb it might be more useful to make this a react hook, otherwise we have to always get the context before using this function
function useResolveResponsiveProp(value?: ResponsiveProp<T>) {
const { getSnapshot } = useContext(MediaQueryContext);
if (!value || !isResponsiveValue(value)) return value;
const fallback = value.base ?? value.phone ?? value.tablet ?? value.desktop;
if (!getSnapshot) return fallback;
if (typeof value.phone !== 'undefined' && getSnapshot(media.phone)) return value.phone;
if (typeof value.tablet !== 'undefined' && getSnapshot(media.tablet)) return value.tablet;
if (typeof value.desktop !== 'undefined' && getSnapshot(media.desktop)) return value.desktop;
return fallback;
}
There was a problem hiding this comment.
Yeah make sense
haoruikun-cb
force-pushed
the
harryhao/cds-1513-consolidate-listcell-border-props
branch
from
a710a00 to
4020a6e
Compare
haoruikun-cb
force-pushed
the
harryhao/cds-1513-consolidate-listcell-border-props
branch
from
4020a6e to
0c1c779
Compare
haoruikun-cb
deleted the
harryhao/cds-1513-consolidate-listcell-border-props
branch
2 participants
What changed? Why?
Updated Cell border handling so all border-related props are applied to the same inner container as
borderRadius(web + mobile), and added focused border customization stories for both platforms. This resolves a customer-reported issue where borders were hard to configure becauseborderedapplied to the outer Box whileborderRadiusapplied to the inner Pressable, causing inconsistent results (see conversation: https://coinbase.slack.com/archives/C05H922EYP7/p1768246943588929).Refactored Interactable to accept responsive
borderColorand added coverage for breakpoint-based resolution. This improves Interactable/Pressable parity with other responsive props and makes border styling consistent across breakpoints.Root cause (required for bugfixes)
UI changes
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