Revert #55219 fix/block-settings-origins #58951
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
ajlende
changed the title
|
Size Change: +240 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Ok, it seems in the discussion #55219 most people thinking the revert is the way to go for now. cc @matiasbenedetto how do you feel about it? Do you think this can work for the font library as well? Thanks |
youknowriad
requested review from
pbking,
matiasbenedetto,
andrewserong,
t-hamano and
aaronrobertshaw
youknowriad
added
the
[Type] Bug
|
I added #58753 to the list of Issues that this PR would resolve. |
getdave
added
the
Backport to WP 6.7 Beta/RC
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @dabowman, @bgardner. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
MaggieCabrera
previously approved these changes
Feb 13, 2024
There was a problem hiding this comment.
I tested this using the theme.json file in #52200 and the font sizes of the theme were showing correctly. I then added an extra font size using one of the slugs for preset core font sizes and its value was overridden correctly too.
Can confirm this is fixed too |
|
Onew problem with this PR is the impact it has on Font Family. If I have a theme with 2 active fonts, and then I install a new one using the Font Library, I now lose access to the theme's fonts in the Post Editor. For example here I am using a theme that has two fonts - Crimson Text and Roboto Mono, and I have installed Aboreto from Google Fonts. In trunk I see this: But with this PR I see: I think the way forward is probably going to be to make an exception for Font Family until we can find a better solution for all cases. |
Yes, reverting #55219 means reproducing #55011, which #55219 originally wanted to solve. We need to add logic to manually merge font families from each origin, rather than retrieving them from the merged origin. I'd like to look into it, but if it doesn't seem to be ready in time for WP6.5 Beta1, I might be able to do a follow-up. |
|
@scruffian @getdave I've restored the behavior for font families but also found that the merge function was used for shadow presets, so I kept this like trunk as well. I also removed the two expected functions mergeOrigins and hasMergedOrigins. Both of these functions seemed like early abstraction that are not clear enough. We might want to revisit if/when we unify all the panels. |
|
I'd appreciate more testing here. |
|
@getdave fixed I think, another round of testing? |
|
@youknowriad My understanding is that there is no need to split the origin when it comes to font size. f153736#diff-cd3303ad473e0bbfc9d92758c62c8f4295b31a0bb8283a3e4445c38fb73c0f4dR233-R235 |
Regarding this, I have confirmed that both theme fonts and user-installed fonts are displayed correctly at both block level and global styles. |
|
@t-hamano For me, since font sizes is within the |
|
Great to see this land. Thanks for everyone's work here.
I do think this aspect is important. |
|
I think this PR is causing a regression. I'm unable to open the typography panel. The editor breaks. I'm getting this running Gutenberg trunk:
|
|
@matiasbenedetto do you have reproduction steps? It's working for me :) |
Yes, use 'empty' theme and follow the video steps. This little PR should solve it. 2024-02-15.14-57-22.mp4 |
|
The same logical error repeats in shadows, too. I'll add the fix in #59101 |
matiasbenedetto
added a commit
that referenced
this pull request
Feb 15, 2024
|
@t-hamano I see that you restored the two exported functions "overrideOriginals" and "hasOrigin". I had explicitly removed these. See this comment #58951 (comment) because IMO, these are bad factorizations. It gives the impression that all the customization tools should work that way but it shouldn't, I prefer to avoid the indirection here personally. Any particular reason for having these functions? |
|
@youknowriad As mentioned in this comment, I thought it was necessary to clearly indicate "merge" and "override" at this time, so I restored the function. Instead, I made the comments more detailed to communicate what the function does to help with refactoring in the future. gutenberg/packages/block-editor/src/store/get-block-settings.js Lines 114 to 121 in 0e899f7 gutenberg/packages/blocks/src/api/constants.js Lines 274 to 277 in 0e899f7 |
aristath
added a commit
that referenced
this pull request
Feb 16, 2024
* trunk: (78 commits) Components: Use `Element.scrollIntoView()` instead of `dom-scroll-into-view` (#59085) DataViews: Correctly display featured image that don't have image sizes (#59111) Elements: Fix block instance element styles for links applying to buttons (#59114) Allow editing of image block alt and title attributes in content only mode (#58998) Add toggle for grid types and stabilise Grid block variation. (#59051) Global Styles: fix console error in block preview (#59112) Navigation: Avoid using embedded records from fallback API (#59076) Refactor responsive logic for grid column spans. (#59057) Editor: Unify Mode Switcher component between post and site editor (#59100) Move StopEditingAsBlocksOnOutsideSelect to Root (#58412) Fix logic error in #58951 (#59101) Block-editor: Auto-register block commands (#59079) Fix small typo in rich text reference guide (#59089) Components: Add lint rules for theme color CSS var usage (#59022) Enter editing mode via Enter or Spacebar (#58795) Updating Storybook to v7.6.15 (latest) (#59074) CustomSelectControl (v1 & v2): Fix errors in unit test setup (#59038) Add stylelint rule to prevent theme CSS vars outside of wp-components (#59020) ColorPicker: Style without accessing InputControl internals (#59069) Pattern block: batch replacing actions (#59075) ...
|
@t-hamano So it's not that important but for me there are two things here:
The other thing is that |
|
I see. I would like to submit a PR for refactoring, should we backport it to WP6.5? There should be no problem if we refactor correctly, but I'm a little concerned that it might introduce new regressions to WP6.5. |
|
@t-hamano A PR that we don't backport is fine too :) |
|
I just cherry-picked this PR to the cherry-pick-beta-2 branch to get it included in the next release: f2a5323 |
getdave
added a commit
that referenced
this pull request
Feb 20, 2024
Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: widoz <wido@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: iamtakashi <iamtakashi@git.wordpress.org> Co-authored-by: juanfra <juanfra@git.wordpress.org> Co-authored-by: hanneslsm <hanneslsm@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org>
getdave
pushed a commit
that referenced
this pull request
Feb 20, 2024
getdave
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
added a commit
that referenced
this pull request
Feb 20, 2024
Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: widoz <wido@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: iamtakashi <iamtakashi@git.wordpress.org> Co-authored-by: juanfra <juanfra@git.wordpress.org> Co-authored-by: hanneslsm <hanneslsm@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org>
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
creativecoder
added
the
Global Styles
|
Note that as part of curating the Gutenberg 17.8 changelog, I added the label Global Styles to this PR to better categorize it. Please let me know if there is a different label that would be a better fit. |
This was referenced May 10, 2024
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?
Revert for #55219.
Why?
Partially fixes #52200
Closes #58753
In draft because the code that needs to be reverted is now used in more places, and we need to confirm if it needs to be reverted here or in the specific place that it was changed before.
How?
Make the
mergeOriginsfunction work like it did before, by just returning the highest priority value instead of flattening them in one array. We may want to do some additional renames like suggested in #55219 (comment).Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
TODO