Hotfix wrong override when value is not defined in breakpoint #6969
Conversation
|
@AntoineDuComptoirDesPharmacies can you run |
Done, thank you for this tip ! |
|
looking into the jest test now and seeing what going on.. for the bundle size error can you go here and change |
|
@AntoineDuComptoirDesPharmacies I beleive your |
|
You can also try creating a new fresh branch to see if that resolves the snapshot issues |
…k and update yarn + max package size"This reverts commit 2c249a2. Revert "Run yarn jest -u as requested by @britt6612." This reverts commit 8ac167c.
Thank you, I revert the commits containing snapshot update and modify the only snapshot who have changed. |
| const breakpoint = | ||
| responsiveBreakpoint && theme.global.breakpoints[responsiveBreakpoint]; | ||
| const breakpoint = getBreakpointStyle(theme, responsiveBreakpoint); | ||
| const metric = theme.global.edgeSize[data] || data; |
There was a problem hiding this comment.
Behavior looks great. Two small naming suggestions would be to align the variable names with the concept of CSS value. Metric was a little bit confusing at first:
- metric --> value
- responsiveMetric --> responsiveValue
soft opinion.
| @@ -579,12 +579,6 @@ exports[`Tip themed 1`] = ` | |||
| animation-delay: 0.01s; | |||
| } | |||
|
|
|||
| @media only screen and (max-width:768px) { | |||
There was a problem hiding this comment.
I'm trying to understand why this margin goes away with these changes. Can you explain?
What does this PR do?
This PR tend to avoid generating a wrong CSS override when value is not known in theme breakpoint (Same as for
gap, etc...)Where should the reviewer start?
Read the linked issue to see the difference between both Codepens.
What testing has been done on this PR?
Manual tests only
How should this be manually tested?
Use Developper console with the new version to figure out that CSS override have not been generated for unknown value.
Then add a 'small' breakpoint with distinct edgeSize value to see the override being generated.
Do Jest tests follow these best practices?
No Jest tests.
Any background context you want to provide?
What are the relevant issues?
Issue #6968
Screenshots (if appropriate)
N/A
Do the grommet docs need to be updated?
No
Should this PR be mentioned in the release notes?
Maybe, depending if a lot of people get a specific expected behavior with this bug.
Is this change backwards compatible or is it a breaking change?
It should be backwards compatible, except if some users get a specific behavior getting the raw CSS value inserted and being different than the value defined in global theme.