-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fluid typography: ensure max viewport width is used in the editor #51146
Merged
ramonjd
merged 4 commits into
trunk
from
fix/fluid-font-size-layout-wide-size-not-being-passed
Jun 1, 2023
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid backwards compat problems I'm trying to keep the function signature the same, hence the gymnastics here.
Because we support
fluid: true
andfluid: {...values}
, we have to ensure the param object is correctly set so that we can know if fluid typography is enabled or not.Also, we need to pass
settings?.layout?.wideSize
if available only when fluid font sizes are enabled. 😵The alternative could be a third argument to the
getTypographyFontSizeValue()
function, e.g.,isFluidEnabled
.Or add a prop and pass the following second argument to
getTypographyFontSizeValue
:and move the logic there? I think that might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit of gymnastics to maintain back compat, but I actually don't mind it how it's written as-is, as it means that the calling code is responsible for ensuring that the spread
settings.typography.fluid
overrides themaxViewPortWidth
set via the layoutwideSize
, and thegetTypographyFontSizeValue
doesn't have to work too hard to figure it out.That said, adding
layoutWideSize
toTypographySettings
could be a clever way to do it, then it'd be fairly straightforward to uselayoutWideSize
only ifmaxViewPortWidth
doesn't exist.Whatever you think works best, I don't think I mind either way, personally!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all that familiar with this part of the codebase so take this with a large grain of salt.
What if we extract all of the code in
getTypographyFontSizeValue
that transforms thesettings.typography
object into an object that's suitable for passing togetComputedFluidTypographyValue
into a new function?That way
valueFunc
can setmaximumViewPortWidth
on a normalised object and not have to deal with the intricacies of parsing thesettings.typography.fluid
boolean / object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think what you have is that bad though. This suggestion is for if it's really bugging you 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
yes
YES!
packages/block-editor/src/components/global-styles/typography-utils.js
exists for a reason 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping @noisysocks and @andrewserong
I might merge as is and mark this suggestion for a follow up, since it might come in handy down the track.