Use all the settings origins for a block that consumes paths with merge. #55219
Conversation
Co-authored-by: Jason Crist <[email protected]>
matiasbenedetto
added
[Type] Bug
|
Size Change: +5 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
@matiasbenedetto it appears this PR might have introduced a regression as outlined by @t-hamano over in #55744 (comment). Some existing documentation hints that presets from an origin are merged differently. Was there some additional logic needed for this change? |
|
@aaronrobertshaw sorry, I didn't see your ping before. I'm coming back here from a discussion started in #58753. My rationale: I think #55219 is faithful to the idea that all origins of From that perspective, this PR is valid, and the previous behavior, where the availability of 'custom' settings suppressed the ability to select 'theme' ones, and the availability of 'theme' settings suppressed the ability to select 'default' ones, can be considered a bug. The idea behind this PR was to enable users to select all the settings rendered to the frontend. Said behavior matches in the UI the actual functionality of the |
|
@matiasbenedetto I think that rationale makes sense indeed. We do indeed show both custom colors palettes and theme color palettes at the same time. But It seems it may have been specific to colors thus the impact we're seeing in all other settings. So I just want to make sure we're not introducing bugs. |
|
For instance, do we also inject all font sizes in the frontend from all origins? |
Not only color palettes, but i.e., font families from all origins, are also rendered to the frontend.
Yes, all the font sizes are also rendered to the frontend. |
Even if it is a bug, the previous behavior has been valid for a long time and is pervasive among users. This change will have a significant impact on the editor's UI regarding colors, font size, and spacing presets. Therefore, I believe we should have fully investigated the impact of this PR on all UIs with presets and identified any necessary logic changes to those UIs. Also, if this PR is to be maintained, I think it is necessary to at least publish the scope of influence as a Dev Note. Additionally, this issue also affects spacing presets, as reported in this comment. However, settings like |
|
Thanks for the discussion here, folks! +1 to the idea of adding a Dev Note, assuming this PR remains in place for 6.5. It does sound like a useful feature to combine the values from all the origins. At the same time, I can imagine running into a number of challenges in different bits of UI, as Aki mentions. The fix for the Cover block placeholder colors and the text colors in the text highlighter over in #58869 attempts to fix things for those cases while preserving the intent behind the change here 🤞 |
|
At a glance, the behaviour to have all the settings form different origins merged, makes sense, and is how I initially thought theme.json/global styles would work. As @t-hamano pointed out, that hasn't been the case for a significant period of time though. So I'm not sure I agree with breaking multiple other areas in the UI in order to fix one thing elsewhere of comparable severity. Is it possible to revert the change until we can identify its full impact and have any other required patches in the works? Or, are there other new features now depending on this that mean we just have to accept the breakage? Also, how does the merging of all origin values play with theme style variations? My limited understanding there is that theme style variations effectively have their settings and styles copied to the user origin. From there, the theme style variation's color palettes etc were supposed to completely override or replace those defined in the primary theme.json file. On a related note, some of the recent feedback on the extended block style variations was that the block style variations defined in a theme style variation should completely replace those defined in the primary theme.json (or standalone partial files) rather than be merged. Apologies if I'm missing something here. |
|
Personally, I feel that the disadvantages of merging all origins outweigh the advantages at this point, so I'm leaning toward reverting this PR. If I understand correctly, I think that after reverting this PR, we can merge the origins in stages. I would like to suggest the following approach, please let me know your opinions. 🙏 Tasks to backport to WordPress6.5
Tasks required after WordPress 6.5
|
|
I agree with @t-hamano about reverting this PR. The change in the UI is dependent on the I don't think that was known or was communicated clearly when the typography stack was being implemented. In #58409, I've been working on implementing the pattern we have for earlier presets for font sizes. Hopefully the same form of migration can be used for font families and spacing sizes so we can handle all migrations in one theme.json version bump. Reverting this PR will make those migrations easier. |
|
I was just on a call with @annezazu and @bgardner about an issue with the UI for font sizes being incorrect (#57889 (comment)). I opened #58951 to revert this, and it seemed to fix that issue. Having font sizes not reflect what is going on in the frontend is a critical bug for 6.5, so that is another reason to revert this for now. |
|
Cross-linking for greater visibility: #52200 (comment) |
Co-authored-by: ajlende <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: carolinan <[email protected]> Co-authored-by: justintadlock <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: widoz <[email protected]> Co-authored-by: colorful-tones <[email protected]> Co-authored-by: iamtakashi <[email protected]> Co-authored-by: juanfra <[email protected]> Co-authored-by: hanneslsm <[email protected]> Co-authored-by: richtabor <[email protected]>
Co-authored-by: ajlende <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: carolinan <[email protected]> Co-authored-by: justintadlock <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: widoz <[email protected]> Co-authored-by: colorful-tones <[email protected]> Co-authored-by: iamtakashi <[email protected]> Co-authored-by: juanfra <[email protected]> Co-authored-by: hanneslsm <[email protected]> Co-authored-by: richtabor <[email protected]>
Co-authored-by: ajlende <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: MaggieCabrera <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: carolinan <[email protected]> Co-authored-by: justintadlock <[email protected]> Co-authored-by: annezazu <[email protected]> Co-authored-by: widoz <[email protected]> Co-authored-by: colorful-tones <[email protected]> Co-authored-by: iamtakashi <[email protected]> Co-authored-by: juanfra <[email protected]> Co-authored-by: hanneslsm <[email protected]> Co-authored-by: richtabor <[email protected]>
What?
Use all the settings origins for a block that consumes paths with merge.
Why?
Because we are not displaying all the available settings if there are more than one origin in use.
How?
By concatenating all the origins
Testing Instructions
Please take a look at the screencast below to see how you can test this using the editor UI.
Screenshots or screencast
This change is not exclusive to font families and is not specific about the font library, I'm just showing the usage of the font library because it's easy to test the change using the UI because the installed fonts are in a different key than the theme ones.
2023-10-10.15-58-32.mp4
Fixes #55011
Co-authored-by: Jason Crist [email protected]