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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ae311f
fix: top padding not removed
andrefcdias Feb 10, 2022
1728abb
Change files
andrefcdias Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@fluentui-react-card-7f26a0be-e17d-4051-bc1f-69dab29d49f6.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "fix: top padding issues with CardPreview",
"packageName": "@fluentui/react-card",
"email": "[email protected]",
"dependentChangeType": "patch"
}
16 changes: 4 additions & 12 deletions packages/react-card/src/components/Card/useCardStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,19 @@ export const cardClassName = 'fui-Card';
*/
const useStyles = makeStyles({
root: {
display: 'flex',
flexDirection: 'column',
display: 'block',
...shorthands.overflow('hidden'),

boxShadow: tokens.shadow4,
color: tokens.colorNeutralForeground1,
backgroundColor: tokens.colorNeutralBackground1,

// Size: medium
// TODO: Validate if we should use a token instead + the unit of said token
// TODO: Explore alternate way of applying padding
...shorthands.padding('12px'),
...shorthands.gap('12px'),
...shorthands.borderRadius(tokens.borderRadiusMedium),

[`> .${cardPreviewClassName}`]: {
marginLeft: '-12px',
marginRight: '-12px',
'&:first-child': {
marginTop: '-12px',
},
[`> *:not(.${cardPreviewClassName})`]: {
// Size: medium
...shorthands.margin('12px'),
Comment on lines +23 to +25
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.

Copy link

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?

},
},

Expand Down
4 changes: 1 addition & 3 deletions packages/react-card/src/components/CardPreview/useCardPreviewStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { shorthands, makeStyles, mergeClasses } from '@griffel/react';
import { makeStyles, mergeClasses } from '@griffel/react';
import type { CardPreviewState } from './CardPreview.types';

export const cardPreviewClassName = 'fui-CardPreview';
Expand All @@ -9,8 +9,6 @@ export const cardPreviewClassName = 'fui-CardPreview';
const useStyles = makeStyles({
root: {
position: 'relative',
// TODO: Explore alternate way of applying padding on parent Card
...shorthands.margin('0', '-12px'),

'> *': {
display: 'block',
Expand Down