Global Styles: Allow font sizes to be overridden #57692
Closed
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
scruffian
added
the
Global Styles
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Size Change: +7 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
ajlende
reviewed
Jan 9, 2024
There was a problem hiding this comment.
Two things:
- Should the array reversal be done in
mergeOrigins()instead? How are the other global styles handling this? Are other things bugged too? - It seems wasteful to just reverse the array and output everything when we already know that the first one output will always be unused. It's only a handful of bytes that we're saving, but it doesn't seem that much harder to just remove the duplicates.
| @@ -104,7 +104,7 @@ function getUniqueFontSizesBySlug( settings ) { | |||
| const fontSizes = settings?.typography?.fontSizes; | |||
| const mergedFontSizes = fontSizes ? mergeOrigins( fontSizes ) : []; | |||
| const uniqueSizes = []; | |||
| for ( const currentSize of mergedFontSizes ) { | |||
| for ( const currentSize of mergedFontSizes.reverse() ) { | |||
There was a problem hiding this comment.
I wouldn't use .reverse() like this. It mutates the array in-place. Having read through mergeOrigins(), it looks like it returns a new array, so it wouldn't have side-effects here, but it would be better to attach the .reverse() to mergeOrigins( fontSizes ).reverse() so the relationship between them is more clear.
Alternatively, if you don't want to mutate the original array, [].toReversed() has good browser support these days at the cost of more memory usage.
|
I dont think this is the right approach. |
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?
Allow themes to override the default font sizes.
WIP
Why?
When a theme provides custom font sizes that use the same slugs as the default, the theme ones should override the defaults.
How?
The way that this works is not the right way to do it.
Font sizes get merged from "default" and "theme" into a "custom" attribute. This is then made unique in order, so I have reversed the order so that the theme settings win. I think the way we should do this is to provide both "default" and "theme" to the control and allow it to do a merge based on the key rather than just the order.
Testing Instructions
Screenshots or screencast