feat(react-card): add size prop
#22915
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0a0b94c:
|
📊 Bundle size report
Unchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1282 | 1286 | 5000 | |
| Button | mount | 790 | 815 | 5000 | |
| FluentProvider | mount | 2362 | 2371 | 5000 | |
| FluentProviderWithTheme | mount | 396 | 444 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 368 | 331 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 440 | 444 | 10 | |
| MakeStyles | mount | 2020 | 1988 | 50000 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: ecb991eff272bc8ed5d3f23ccf21e2b8cbf0dc6b (build) |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
andrefcdias
changed the title
size propsize prop
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| // TODO: Extract this to separate styles when tackling the `size` prop | ||
| ...shorthands.margin('12px'), | ||
| ...shorthands.padding(`var(${cardSizeVar})`), | ||
| ...shorthands.gap(`var(${cardSizeVar})`), |
There was a problem hiding this comment.
have you considered passing the size down the react tree through context so that the relevant component can own the style ? It will also be easier to override for users because they are less specific
There was a problem hiding this comment.
Yes, we've considered that possibility before, as well as other alternatives to spacing, but chose to go with the simplest solution for now as conversations kept delaying implementation.
Each solution has its own pros and cons. For that one, users who add non Card components to a Card would get a bad DX as they get no padding by default and would need to consume a context to apply something as trivial as a padding. Another problem is the fact that we can't have doubled paddings in between elements and flex displays do not allow for collapsible margins, which would mean a complex logic to apply the proper padding.
I'm 100% open to suggestions to these cons however.
There was a problem hiding this comment.
Oh this is also for non-card components ? I don't see this var being used in any styles targeting non-card components here
Is the intended ussage something like this?
import { makeStyles, cardSizeVar } from '@fluentui/react-components`;
const useStyles = makeStyles({
cardPadding: {
...shorthands.padding(`var(${cardSizeVar})`)
}
})
function NonCardComponent() {
const styles = useStyles();
return <div className={styles.cardPadding} />
}There was a problem hiding this comment.
Not particularly but it's also a possibility we open for our users, especially if we end up not using gap.
However, the reason we put this in a var is to simplify any overwriting of the value as changing the var will affect both padding and gap, and these two need to be aligned.
There was a problem hiding this comment.
and these two need to be aligned
This point makes sense to me. I do wonder if we will consider this cssVar a part of our public API, once feature engineers do a devtools inspect and see it, they can certainly use it in overrides. If we ever rename/remove this var it could break those overrides
There was a problem hiding this comment.
I see that slider already does this, if we will stick with the cssvar approach we should export it for users at least
There was a problem hiding this comment.
Good point. My approach was not to export until we're sure we want to for the reasons you mentioned (avoiding misuse or name changes).
Thanks for sharing the slider example though. I'll align the usage and export it to avoid possible problems.
| [`> *:not(.${cardPreviewClassNames.root})`]: { | ||
| // TODO: Extract this to separate styles when tackling the `size` prop | ||
| ...shorthands.margin('12px'), | ||
| ...shorthands.padding(`var(${cardCSSVars.cardSizeVar})`), |
There was a problem hiding this comment.
😍 using css vars the rite way yay !
| @@ -177,6 +201,7 @@ export const useCardStyles_unstable = (state: CardState): CardState => { | |||
| state.root.className = mergeClasses( | |||
| cardClassNames.root, | |||
| styles.root, | |||
| sizeMap[state.size], | |||
New Behavior
Added the
sizeproperty which will control:Related Issue(s)
#19336