Fluid typography: rename viewport variables #53082
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
One day the max/min viewport widths will be configurable in theme.json, let's make sure we start off with consistency now.
ramonjd
added
[Type] Code Quality
This was referenced Jul 28, 2023
|
Size Change: +15 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
andrewserong
approved these changes
Jul 28, 2023
There was a problem hiding this comment.
This looks good to me.
Technically, getComputedFluidTypographyValue is exported publicly, but in practice, I think a few things mitigate any impact of changing the props:
- As you mention, these values aren't yet configurable in
theme.json - This change shouldn't cause any errors as they're optional params in the object passed to
getComputedFluidTypographyValue. If for some reason there was usage out in the wild with the old params, the code in this PR uses sensible defaults, so I imagine the impact would be very minimal - From a plugin search, it seems that the only other plugin with installs using this function is one that ships a build of the block editor JS, so I don't think this would affect anything there
So all up, I think this change sounds good, and I don't think it'd be worth adding in a deprecated notice for the old props. I smoke tested this change in TT3, and everything appears to still be working correctly, so this LGTM! ✨
I didn't consider that, thanks for raising. Do you think we should support it? Or maybe deprecate it: deprecated( '`minumumViewPortWidth` and `maximumViewPortWidth` have been renamed to `minumumViewportWidth` and `maximumViewportWidth`, {
since: '6.4',
} ); |
Oh, just realized you wrote this. Thanks for thinking about it 🙇🏻 |
No worries!
I think the deprecated way would be the "correct" way to handle it. In this case, IMHO, the deprecated approach is probably not necessary, due to the low likelihood that these props were being used anywhere. Happy to support whichever way you'd like to go with this PR, though! 🙂 |
Oh my gosh, I did not notice that typo 😂 Thanks for fixing up and landing this! |
You shouldn't be my spell checker all the time! 😄 Let's just switch everything to Italian or German or some other phonetically-written language |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Changing
viewPorttoviewportin the fluid typography context.Why?
Viewport is one word, so we shouldn't camelCase it.
One day the max/min viewport widths will be configurable in theme.json, let's make sure we start off with consistency now.
How?
Search, find, replace.
Testing Instructions
The tests should pass. Smoke test in 2023 to make sure fluid typography works 😄