Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(react-card): top padding not removed for CardPreview #21685

Merged
merged 2 commits into from
Feb 10, 2022
Merged

fix(react-card): top padding not removed for CardPreview #21685

merged 2 commits into from
Feb 10, 2022

Conversation

andrefcdias
Copy link
Contributor

Current Behavior

image

New Behavior

image

Related Issue(s)

Fixes #21649

@andrefcdias andrefcdias mentioned this pull request Feb 10, 2022
37 tasks
@codesandbox-ci
Copy link

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 1728abb:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-card
Card - All
48.512 kB
14.352 kB
47.867 kB
14.195 kB
-645 B
-157 B
react-card
Card
43.987 kB
13.188 kB
43.56 kB
13.004 kB
-427 B
-184 B
react-card
CardPreview
7.806 kB
3.357 kB
7.588 kB
3.255 kB
-218 B
-102 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-card
CardFooter
7.615 kB
3.23 kB
react-card
CardHeader
8.893 kB
3.675 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
165.744 kB
46.848 kB
react-components
react-components: FluentProvider & webLightTheme
32.479 kB
10.625 kB
🤖 This report was generated against d5a34ee8ba3e9204aeb99214321045c3d6b786fa

Comment on lines +23 to +25
[`> *:not(.${cardPreviewClassName})`]: {
// Size: medium
...shorthands.margin('12px'),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All design systems seem to opt for an individual padding of the contents of cards, instead of a global padding. I went with collapsible margins applied to any content but CardPreview so that we keep the control over the values in the Card component, given that the size prop implemented in the future will control this value.
This should also be easy to override by the user on any custom content that they add to the Card, since the user is aware of which size they will be using.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another solution would be to have a Card context provided for the components to know the values to pad it with, but I'd rather keep a simpler solution and something that works out of the box for anyone who adds custom content.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change adds margin:12px on CardHeader - is that intentional - if we need to suppress it, we need to either supply a rule with higher specificity or via !important? How does this relate to fitted attribute on cardHeader in north-star repro?

@size-auditor
Copy link

size-auditor bot commented Feb 10, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: d5a34ee8ba3e9204aeb99214321045c3d6b786fa (build)

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 845 860 5000
BaseButton mount 966 963 5000
Breadcrumb mount 2749 2713 1000
ButtonNext mount 470 499 5000
Checkbox mount 1572 1549 5000
CheckboxBase mount 1393 1396 5000
ChoiceGroup mount 4854 4896 5000
ComboBox mount 1006 1014 1000
CommandBar mount 10378 10448 1000
ContextualMenu mount 8797 8708 1000
DefaultButton mount 1175 1222 5000
DetailsRow mount 3892 3826 5000
DetailsRowFast mount 3831 3812 5000
DetailsRowNoStyles mount 3747 3597 5000
Dialog mount 3157 3123 1000
DocumentCardTitle mount 188 182 1000
Dropdown mount 3266 3290 5000
FluentProviderNext mount 1895 1941 5000
FluentProviderWithTheme mount 176 153 10
FluentProviderWithTheme virtual-rerender 122 112 10
FluentProviderWithTheme virtual-rerender-with-unmount 215 229 10
FocusTrapZone mount 1837 1879 5000
FocusZone mount 1912 1890 5000
IconButton mount 1863 1865 5000
Label mount 397 390 5000
Layer mount 3076 3132 5000
Link mount 533 529 5000
MakeStyles mount 1765 1702 50000
MenuButton mount 1565 1533 5000
MessageBar mount 2087 2148 5000
Nav mount 3373 3399 1000
OverflowSet mount 1168 1182 5000
Panel mount 2538 2554 1000
Persona mount 890 894 1000
Pivot mount 1484 1477 1000
PrimaryButton mount 1321 1328 5000
Rating mount 7796 7815 5000
SearchBox mount 1442 1371 5000
Shimmer mount 2640 2596 5000
Slider mount 2107 2005 5000
SpinButton mount 5113 5173 5000
Spinner mount 470 480 5000
SplitButton mount 3265 3250 5000
Stack mount 569 552 5000
StackWithIntrinsicChildren mount 2467 2405 5000
StackWithTextChildren mount 5482 5404 5000
SwatchColorPicker mount 11592 11918 5000
TagPicker mount 2738 2671 5000
TeachingBubble mount 13258 13632 5000
Text mount 483 453 5000
TextField mount 1454 1412 5000
ThemeProvider mount 1252 1229 5000
ThemeProvider virtual-rerender 673 636 5000
ThemeProvider virtual-rerender-with-unmount 1975 1936 5000
Toggle mount 866 862 5000
buttonNative mount 159 167 5000

Perf Analysis (@fluentui/react-northstar)

⚠️ No perf measurements available

@andrefcdias andrefcdias requested a review from Hotell February 10, 2022 13:16
@andrefcdias andrefcdias merged commit 4a28b5a into microsoft:master Feb 10, 2022
@andrefcdias andrefcdias deleted the fix-card-top-padding branch February 10, 2022 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

react-card: Top padding not removed for CardPreview when used as a first child of a Card
4 participants