Skip to content

react-card - CardPreview top padding fix #20030

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

Merged
theerebuss merged 4 commits into from
Oct 4, 2021
Merged

react-card - CardPreview top padding fix #20030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01a68a1
Add margin adjustmentes with fixed className
Sep 29, 2021
955596f
Change files
Sep 29, 2021
f2cb3b7
Update snapshots
Sep 29, 2021
7cf0fda
Merge branch 'master' into card-preview-padding
Oct 1, 2021
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-eef6d228-043e-411b-87c8-94969a3e7cab.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "CardPreview removes own top margin when the first element in a Card",
"packageName": "@fluentui/react-card",
"email": "andredias@microsoft.com",
"dependentChangeType": "patch"
}
2 changes: 1 addition & 1 deletion packages/react-card/src/components/Card.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export const GridviewCard = () => (
maxWidth: '368px',
}}
>
<CardPreview style={{ marginTop: '-12px' }}>
<CardPreview>
<img src="./sales_template.png" alt="Preview of a sales slide deck" />
</CardPreview>
<CardHeader
Expand Down
8 changes: 8 additions & 0 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 @@ -20,6 +20,14 @@ const useStyles = makeStyles({
padding: '12px',
gap: '12px',
borderRadius: theme.borderRadiusMedium,

'> .fluentui-react-card-preview': {
marginLeft: '-12px',

@tringakrasniqi tringakrasniqi Sep 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it is not a very good practice to use negative values for margins, maybe (if it is possible) removing the margin from the above component could be better?

Also, the values are fixed, could there be a possibility that the card might not look as it should because of this?

@theerebuss theerebuss Sep 30, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regarding negating the margins, I'm also not a fan of this solution but I think it's a compromise between elegance and utility.
I had already discussed this topic with @varholak-peter about having the padding instead on the child components (CardHeader, CardPreview, etc), which would potentially remove this issue as each component governs itself. However, I see 2 issues with this:

  • If our users use Card and not the child components, the component would not follow the Fluent guidelines out of the box.
  • Whenever we have the size prop on Card (according to the current spec), we'll need to change this padding as it won't always be the fixed 12px. This means that we'd probably need to provide some CardContext for the inner components to consume and be compliant.
    Given these 2 constraints, I think the less elegant negative margin solution is the better option as it will guarantee it's Fluent by default, allow us to control the padding from the parent (whenever we have size) and avoid duplicate code on the inner components.
    Regardless of what we choose, we felt it was a discussion that needed to happen more in depth and opted for this just to fast track it for now.

As for the mention about fixed values, 12px matches the, eventual, default size for card which is medium. Given that the size prop won't be implemented for phase 1, we went with a fixed value for now.

marginRight: '-12px',
'&:first-child': {
marginTop: '-12px',
},
},
}),

interactive: theme => ({
Expand Down
2 changes: 1 addition & 1 deletion packages/react-card/src/components/CardPreview/__snapshots__/CardPreview.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`CardPreview renders a default state 1`] = `
<div>
<div
class=""
class="fluentui-react-card-preview"
>
Default CardPreview
<div
Expand Down
2 changes: 1 addition & 1 deletion packages/react-card/src/components/CardPreview/useCardPreviewStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const useStyles = makeStyles({
*/
export const useCardPreviewStyles = (state: CardPreviewState): CardPreviewState => {
const styles = useStyles();
state.className = mergeClasses(styles.root, state.className);
state.className = mergeClasses('fluentui-react-card-preview', styles.root, state.className);

if (state.logo) {
state.logo.className = mergeClasses(styles.logo, state.logo.className);
Expand Down