Skip to content

fix(react-card): refactor CardHeader to use grid display#23449

Merged
theerebuss merged 16 commits into
microsoft:masterfrom
theerebuss:card-header-fix
Jul 4, 2022
Merged

fix(react-card): refactor CardHeader to use grid display#23449
theerebuss merged 16 commits into
microsoft:masterfrom
theerebuss:card-header-fix

Conversation

@theerebuss

@theerebuss theerebuss commented Jun 8, 2022

Copy link
Copy Markdown
Contributor

Current Behavior

Initial implementation of CardHeader was rushed, overlooked and brittle with style overwriting.

New Behavior

  • Using display: grid
  • Not using more explicit "grid-template-areas" as there's no shorthands support in Griffel yet
    Add shorthands support for grid-area griffel#120
  • Structural content slot has been deleted
  • Removed some size restrictions as design specs no longer enforce min/max sizes

Related Issue(s)

Fixes #21909
#19336

Reproduction of issue #21909 with this PRs fix

card with properly rendered header

<Card style={{ width: '800px' }}>
  <CardHeader
    image={<EmojiHand28Regular />}
    header={<Title3>Culture Header</Title3>}
    description={
      <Caption1>
        Include any notes, best practices, or ways to get included that your employees should know about
      </Caption1>
    }
    action={<Button>Action One</Button>}
  />
  <CardPreview>Card Preview Section text</CardPreview>
  <div>
    <ul>
      <li>1</li>
      <li>2</li>
      <li>3</li>
      <li>4</li>
      <li>5</li>
      <li>6</li>
      <li>7</li>
      <li>8</li>
      <li>9</li>
    </ul>
  </div>
</Card>

@codesandbox-ci

codesandbox-ci Bot commented Jun 8, 2022

Copy link
Copy Markdown

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 9f0708b:

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

@fabricteam

fabricteam commented Jun 8, 2022

Copy link
Copy Markdown
Collaborator

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-card
Card - All
66.957 kB
19.042 kB
66.431 kB
19.01 kB
-526 B
-32 B
react-card
CardHeader
10.026 kB
4.067 kB
9.504 kB
3.896 kB
-522 B
-171 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-card
Card
62.113 kB
17.911 kB
react-card
CardFooter
8.461 kB
3.555 kB
react-card
CardPreview
8.562 kB
3.61 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
189.067 kB
52.144 kB
react-components
react-components: FluentProvider & webLightTheme
31.883 kB
10.516 kB
🤖 This report was generated against b4a1586d84a3f1480327d288b128eb102b44ba5a

@size-auditor

size-auditor Bot commented Jun 8, 2022

Copy link
Copy Markdown

Asset size changes

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

Baseline commit: b4a1586d84a3f1480327d288b128eb102b44ba5a (build)

@fabricteam

fabricteam commented Jun 8, 2022

Copy link
Copy Markdown
Collaborator

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1702 1670 5000
Button mount 1275 1238 5000
FluentProvider mount 2276 2318 5000
FluentProviderWithTheme mount 895 942 10
FluentProviderWithTheme virtual-rerender 879 873 10
FluentProviderWithTheme virtual-rerender-with-unmount 930 923 10
MakeStyles mount 2579 2447 50000

@theerebuss theerebuss changed the title chore(react-card): refactor CardHeader to use grid display fix(react-card): refactor CardHeader to use grid display Jun 8, 2022
@theerebuss theerebuss marked this pull request as ready for review June 8, 2022 12:16
@theerebuss theerebuss requested a review from a team as a code owner June 8, 2022 12:16
@tudorpopams tudorpopams requested a review from Hotell June 9, 2022 12:22
@theerebuss theerebuss mentioned this pull request Jun 9, 2022
37 tasks
@Hotell Hotell assigned Hotell and theerebuss and unassigned Hotell Jun 14, 2022
@theerebuss theerebuss requested a review from a team as a code owner June 15, 2022 16:03
export type CardHeaderSlots = {
root: Slot<'div'>;
image: Slot<'div'>;
content?: Slot<'div'>;

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.

Slot no longer required as it existed strictly to make the flex layout work.

Comment on lines +13 to +15
{slots.image && <slots.image {...slotProps.image} />}
<slots.header {...slotProps.header} />
{slots.description && <slots.description {...slotProps.description} />}

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.

Slots now proper optional as designed.

@Hotell Hotell self-requested a review June 17, 2022 08:56
<Card>
<CardHeader
image={<img src={avatarElviaURL} alt="Face of a person" />}
image={{ as: 'img', src: avatarElviaURL, alt: 'Face of a person' }}

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.

what the reason of this syntax?

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.

Avoiding the div wrapper and rendering the img element directly

@theerebuss theerebuss merged commit 317b411 into microsoft:master Jul 4, 2022
@theerebuss theerebuss deleted the card-header-fix branch July 4, 2022 16:47
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.

Card header - text formatting

3 participants