Remove Font style, weight, decoration, and transform presets #27555
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
jorgefilipecosta
requested review from
ajitbohra,
ellatrix,
nerrad,
ntwb,
spacedmonkey,
talldan and
TimothyBJacobs
as code owners
|
Size Change: -39 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
remove/style-weight-decoration-and-transform-presets
branch
2 times, most recently
from
71362e2
to
c5ef934
Compare
6 tasks
There was a problem hiding this comment.
Great work @jorgefilipecosta, thank you for all the effort! 👍
Functionally everything works well for me. The only minor issue I ran into was the application of ariaLabel on the buttons. That throws an error in the console expecting aria-label.
The steps I took to test were:
- Loaded an old post in the editor that had a navigation block relying on the presets
- Checked out this PR
- Reloaded the post in the editor and confirmed it appeared correctly
- Tested all the controls for decoration, transform and weight worked and updated the block as expected.
- Confirmed block looked correct on the frontend
- Tested the deprecation again via pasting the supplied code into the code editor view (this was when I noticed the console error regarding the aria label)
- Added a new navigation block and repeated tests using that.
- Ran the
full-content.test.js- all the deprecation fixtures were green
In terms of the overall reasoning and approach, it makes sense to me but it would be good to have a second set of eyes on this to approve.
packages/block-editor/src/components/text-decoration-control/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/text-transform-control/index.js
Outdated
Show resolved
Hide resolved
|
👋 @jorgefilipecosta This change looks fine to me, given that the values for these properties are a closed set defined by the standard ― they're not an open set of values that would be variable by theme. I've tested that this still works as expected, and that blocks created before this change also work. One thing I'd like your thoughts on is
We also need to keep the documentation for theme.json updated. |
oandregal
reviewed
Dec 11, 2020
| }, | ||
| ]; | ||
|
|
||
| const FONT_WEIGHTS = [ |
There was a problem hiding this comment.
I know this wasn't introduced in this PR but wanted to confirm anyway.
I've cross-checked the names of the numbered values in the standard with what we have here, and this is what I saw:
| Value | Name in the standard | Our current name |
|---|---|---|
| 100 | Thin | Ultralight |
| 200 | Extra Light (Ultra Light) | Thin |
| 300 | Light | Light |
| 400 | Normal | Regular |
| 500 | Medium | Medium |
| 600 | Semi Bold (Demi Bold) | Semibold |
| 700 | Bold | Bold |
| 800 | Extra Bold (Ultra Bold) | Heavy |
| 900 | Black (Heavy) | Black |
Wanted to check if this is fine or whether we want to align with the standard. These being values that core will provide, I'd lean to align their names with the standard. However, I understand that there may be a different "industry standard", so would like to hear what other people think of this. cc @kjellr @jasmussen
There was a problem hiding this comment.
Their names should definitely be industry standard, except for "Normal", which should be "Regular", or it will confuse the interface when selected.
There was a problem hiding this comment.
Yeah, we should be fine with the standard you noted here.
jorgefilipecosta
force-pushed
the
remove/style-weight-decoration-and-transform-presets
branch
2 times, most recently
from
24d58b0
to
7cc6bd2
Compare
jorgefilipecosta
requested review from
chrisvanpatten and
mkaz
as code owners
jorgefilipecosta
force-pushed
the
remove/style-weight-decoration-and-transform-presets
branch
from
7cc6bd2
to
aa0f1f1
Compare
gziolo
added
the
Global Styles
…ndex.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> (+4 squashed commits) Squashed commits: [84854a6ef8] Update packages/block-editor/src/components/text-transform-control/index.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> [c5ef934] Add deprecation fixtures [a57bcb8] Backcompatibility [8c5cc3e] Remove Font style, weight, decoration, and transform presets
jorgefilipecosta
force-pushed
the
remove/style-weight-decoration-and-transform-presets
branch
from
aa0f1f1
to
8a32163
Compare
|
Thank you for the reviews all the feedback related to this PR was applied :) |
jorgefilipecosta
deleted the
remove/style-weight-decoration-and-transform-presets
branch
|
We've missed this proposal #27555 (comment) so I've prepared a PR to address that #27718 |
|
I found this while trying to figure out why our experimental theme project suddenly had no font weights anymore. It took some searching... This has me curious to the reasoning behind removing Font Weights specifically from theme.json. Hence my comment here. :) While I agree that it makes little sense overriding a variable for example saying that "font weight 600 is now 500", that's not all that the theme.json spec provided: What has me, personally, really excited about theme.json is being able to more strongly customize Gutenberg and remove things that don't make sense from a theme point-of-view in the editor. Especially in terms of defaults. Typically, we use a very limited set of font weights in a project. Both because font weights vary depending on the font used, and because we want to limit users choices for font weights. The previous implementation offered us the ability of creating an allow-list of weights. Also, these limited weights mean we typically map them to a more common wording depending on the font family - font weight combination and appearance. For example, bold is strictly defined as 700, but for a given font, it makes more sense for the wording "bold" to be defined as "800". It's "bold" for the user, even though it technically isn't. If you think about a variable as --font-weight--600, it doesn't make sense to ever change. If you think of it as --font-weight--medium, it does. I guess the specific points I'm trying to make are:
|
This was referenced Feb 16, 2021
|
The server-side migration of these attributes via |
|
Follow-up at #35881 to remove these from the |
17 tasks
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.
We added Font weight, style decoration, and transforms styles for now only in the navigation block. These styles contain presets.
The other presets we have are color, gradients, font sizes, and font families.
The colors and gradients presets can be semantic eg: Primary Color, and then the color can be changed.
The font size and font family presets can also be semantic e.g: "Huge", or "Monospace" and then the value itself is changed.
The style, weight, decoration, and transforms are not semantic and the variables/class names generated are the value itself. E.g: It does not make sense to change the font-weight 900 preset to have a value of 800.
We are generating this type of block markup:
Changing any of the preset variable value breaks the expectation given that the variable contains the value in the name.
So using variables is not bringing any advantage, I think it would be better to just use-values.
E.g:
Using values for style attributes is something always supported, and if needed we can always add presets in the future. Removing the presets once shipped into core is not something we can do.
Given that there are even explorations of typographic presets that cover multiple typographic attributes see: #27331, I think it is better to not add all these presets for now.
The other presets we had besides the font family were all of the old, and the font family core is not even shipping any default presets yet.
For now, only the navigation block was using these styles so I'm adding code that makes sure we are back-compatible with markup using the presets.
If these styles are added to more blocks (e.g: non-dynamic blocks) and we continue with presets if we need to remove these presets (e.g: to instead have general typographic presets as the mockups are proposing), being back-compatible will be much harder.
cc: @@aaronrobertshaw
How has this been tested?
I added a navigation block.
I verified I could change Font Appearance, Decoration, and transforms, and the styles worked as expected in the editor and the front end.
In the site editor, I added a navigation block. I went to the global styles, I opened the navigation block panel. I applied Font Appearance, Decoration, and transforms, and the styles worked as expected in the editor and the front end.
I created a post with the following markup of a navigation block relying on presets:
I verified the block looked as expected on the editor and the front end. I verified the block was automatically migrated to the markup without presets once opened on the editor.