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

Unify getCss and expandResponsive #6335

Merged

Conversation

itkrt2y
Copy link
Contributor

@itkrt2y itkrt2y commented Jul 17, 2022

📝 Description

Both getCss and expandResponsive recursively iterate styles.
I fixed not to loop style processing twice for performance.

💣 Is this a breaking change (Yes/No):

No

📝 Additional Information

@changeset-bot
Copy link

changeset-bot bot commented Jul 17, 2022

🦋 Changeset detected

Latest commit: ccd92b4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@chakra-ui/styled-system Patch
@chakra-ui/system Patch
@chakra-ui/props-docs Patch
@chakra-ui/provider Patch
@chakra-ui/react Patch
@chakra-ui/skeleton Patch
@chakra-ui/toast Patch
@chakra-ui/test-utils Patch
create-react-app-ts Patch
gatsby-starter-default Patch
chakra-nextjs Patch
chakra-nextjs-ts Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jul 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
chakra-ui-storybook ✅ Ready (Inspect) Visit Preview Aug 5, 2022 at 6:54AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 17, 2022

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 ccd92b4:

Sandbox Source
create-react-app-ts Configuration

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2022

This pull request introduces 2 alerts and fixes 1 when merging 25451f4 into 19e1532 - view on LGTM.com

new alerts:

  • 2 for Prototype-polluting assignment

fixed alerts:

  • 1 for Prototype-polluting assignment

"color",
"height",
"paddingInlineStart",
"paddingInlineEnd",
"paddingTop",
"paddingBottom",
"@media screen and (min-width: 40em)",
"@media screen and (min-width: 52em)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The results of css() are as follows.
There are some differences, but I think the behavior will not change.

Before

{
  flexDirection: 'column',
  justifyContent: null,
  '@media screen and (min-width: 40em)': {
    justifyContent: 'flex-start',
    paddingInlineStart: 'var(--space-3)',
    paddingInlineEnd: 'var(--space-3)'
  },
  '@media screen and (min-width: 52em)': {
    justifyContent: 'flex-end',
    paddingInlineStart: 'var(--space-4)',
    paddingInlineEnd: 'var(--space-4)'
  },
  color: 'background',
  height: '100%',
  paddingInlineStart: 'var(--space-2)',
  paddingInlineEnd: 'var(--space-2)',
  paddingTop: 'var(--space-4)',
  paddingBottom: 'var(--space-4)'
}

After

{
  flexDirection: 'column',
  color: 'background',
  height: '100%',
  paddingInlineStart: 'var(--space-2)',
  paddingInlineEnd: 'var(--space-2)',
  paddingTop: 'var(--space-4)',
  paddingBottom: 'var(--space-4)',
  '@media screen and (min-width: 40em)': {
    justifyContent: 'flex-start',
    paddingInlineStart: 'var(--space-3)',
    paddingInlineEnd: 'var(--space-3)'
  },
  '@media screen and (min-width: 52em)': {
    justifyContent: 'flex-end',
    paddingInlineStart: 'var(--space-4)',
    paddingInlineEnd: 'var(--space-4)'
  }
}

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2022

This pull request fixes 1 alert when merging 1900f17 into 19e1532 - view on LGTM.com

fixed alerts:

  • 1 for Prototype-polluting assignment

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2022

This pull request fixes 1 alert when merging 7c38c16 into 19e1532 - view on LGTM.com

fixed alerts:

  • 1 for Prototype-polluting assignment

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2022

This pull request fixes 1 alert when merging e2231e0 into 19e1532 - view on LGTM.com

fixed alerts:

  • 1 for Prototype-polluting assignment

@lgtm-com
Copy link

lgtm-com bot commented Jul 17, 2022

This pull request fixes 1 alert when merging 85277e2 into 19e1532 - view on LGTM.com

fixed alerts:

  • 1 for Prototype-polluting assignment

@segunadebayo
Copy link
Member

Great work here @itkrt2y.

Can we include a perf track tool in this PR to visualize the perf gains? Let's use a tool like https://www.npmjs.com/package/cronometro to measure it.

@itkrt2y
Copy link
Contributor Author

itkrt2y commented Jul 23, 2022

@segunadebayo

I tried to introduce cronometro in 11528a9 . (I used node/undici as a reference code)
I'm not sure what kind of benchmark scripts you had in mind, is this commit okay?

Benchmark result

Before (main branch)

$ yarn workspace @chakra-ui/styled-system benchmark
╔══════════════╤═════════╤══════════════════╤═══════════╗
║ Slower tests │ Samples │           Result │ Tolerance ║
╟──────────────┼─────────┼──────────────────┼───────────╢
║ Fastest test │ Samples │           Result │ Tolerance ║
╟──────────────┼─────────┼──────────────────┼───────────╢
║ systemProps  │   10000 │ 155570.77 op/sec │  ± 1.46 % ║
╚══════════════╧═════════╧══════════════════╧═══════════╝

After (this PR)

$ yarn workspace @chakra-ui/styled-system benchmark
╔══════════════╤═════════╤══════════════════╤═══════════╗
║ Slower tests │ Samples │           Result │ Tolerance ║
╟──────────────┼─────────┼──────────────────┼───────────╢
║ Fastest test │ Samples │           Result │ Tolerance ║
╟──────────────┼─────────┼──────────────────┼───────────╢
║ systemProps  │   10000 │ 209751.39 op/sec │  ± 3.97 % ║
╚══════════════╧═════════╧══════════════════╧═══════════╝

@segunadebayo
Copy link
Member

@itkrt2y, that's exactly what I have in mind as well. Thanks for adding that.

I'll review the code shortly and leave any comments.

Copy link
Member

@segunadebayo segunadebayo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Love the extra comments you've added

@segunadebayo segunadebayo merged commit e0913e5 into chakra-ui:main Aug 8, 2022
@itkrt2y itkrt2y deleted the unify-getCss-and-expandResponsive branch August 8, 2022 05:24
anubra266 added a commit to anubra266/chakra-ui that referenced this pull request Sep 16, 2022
@anubra266 anubra266 mentioned this pull request Sep 16, 2022
anubra266 added a commit to anubra266/chakra-ui that referenced this pull request Sep 16, 2022
anubra266 added a commit to anubra266/chakra-ui that referenced this pull request Sep 16, 2022
anubra266 added a commit to anubra266/chakra-ui that referenced this pull request Sep 16, 2022
anubra266 added a commit to anubra266/chakra-ui that referenced this pull request Sep 16, 2022
anubra266 added a commit to anubra266/chakra-ui that referenced this pull request Sep 16, 2022
segunadebayo pushed a commit that referenced this pull request Sep 16, 2022
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.

2 participants