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(breakpoints): multiple responsive style props #141

Conversation

EmilioAiolfi
Copy link
Contributor

@EmilioAiolfi EmilioAiolfi commented Oct 27, 2020

Summary

Hello folks!
There is a bug when passing several responsive style props, the order of the resulting media queries is wrong.
More details on this in issue #119
Example
Code:

<Box 
  pt={{ sm: 0, lg: 60 }}
  pb={{ md: 20, lg: 60 }}
  bg={{ xs: 'blue', md: 'red', lg: 'green', xl: '#000000'}}
>
  hello
</Box>

Screen Shot 2020-10-27 at 6 39 56 PM

Test plan

The idea is to organize the order of @Medias based on the breakpoint values. A function was created to guarantee the order of the breakpoints, without the need to add complexity to the merge function.

Screen Shot 2020-10-27 at 6 39 20 PM

Co-authored-by:  Mario Souto <mario.souto@nubank.com.br>
@gregberge
Copy link
Collaborator

Hello @EmilioAiolfi, thanks for this fix! I have to find a way to test it and I think it could alter performances. All xstyled code is heavily optimized, I see a reduce, etc.. It is probably not as performant as expected. I will give a deeper look to fix it based on your work.

@EmilioAiolfi
Copy link
Contributor Author

EmilioAiolfi commented Oct 28, 2020

Hi @gregberge
I tried to help with this bug by understanding it as something serious, by compromising the use of breakpoints. The problem is to expect a responsibility that I believe would not be the 'util/merge' function. Something like being responsible for @media's orders, and it's something that the merge function really shouldn't know.
About the approach I suggested. it is just one option among so many, but I expected a code review to help with this bug and we managed to solve it.

About this bug is the point of attention about the use of util/merge in reduceBreakpoints and compose

Thanks for @xstyled! I believe in the approach it offers and I insist on trying to help improve it more and more.

Copy link
Collaborator

@gregberge gregberge left a comment

Choose a reason for hiding this comment

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

Small perf improvements.

packages/system/src/style.js Outdated Show resolved Hide resolved
packages/system/src/style.js Outdated Show resolved Hide resolved
@gregberge gregberge merged commit b884076 into styled-components:master Nov 4, 2020
@gregberge
Copy link
Collaborator

Thanks!

@EmilioAiolfi EmilioAiolfi deleted the fix/breakpoint-responsive-styles branch November 4, 2020 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants