[WIP] Try: Add sticky position support for the Group block #38039
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
|
Size Change: +400 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
andrewserong
force-pushed
the
try/add-sticky-position-support-to-group-block
branch
from
31b45ac
to
9c3c0da
Compare
ramonjd
reviewed
Feb 17, 2022
| @@ -23,7 +23,7 @@ | |||
| * | |||
| * @return array The settings to retrieve. | |||
| */ | |||
| function wp_get_global_settings( $path = array(), $context = array() ) { | |||
| function gutenberg_get_global_settings( $path = array(), $context = array() ) { | |||
There was a problem hiding this comment.
Thanks for getting this up so we can have a looksy @andrewserong !!
Looks like a similar approach to the one we suggested over at #38671 (comment) and over at https://github.com/WordPress/gutenberg/pull/38816/files#r808663126, where I'm looking to tackle something similar.
As mentioned over at https://github.com/WordPress/gutenberg/pull/38816/files#r808663126, I'm starting to wonder if we should move changes to this file over to compat/6.0 in a "compatibility preparation branch or something" since we'll have perform some further trickery on it.
By "trickery" I mean updating methods such as {wp|gutenberg}_get_global_stylesheet() to use WP_Theme_JSON_Resolver_Gutenberg instead of WP_Theme_JSON_Resolver, so it picks up the compat/6.0 changes in WP_Theme_JSON_Gutenberg for example.
I'm still lacking a lot of the context and knowledge of the approaches that were taken for 5.9, so maybe @oandregal @jorgefilipecosta @aristath or @youknowriad might have some tips for us.
There was a problem hiding this comment.
I'm looking at this one and I'm going to push changes in the next few hours.
There was a problem hiding this comment.
Hey Andrew, I've heavily edited the commits in this PR to show how can we override global styles code. Sorry about the intrusion. 🙇♂️
Here's what we can do to add new things to the global styles code in the server:
- Update the allowed lists (
PROPERTIES_METADATA,VALID_SETTINGS, etc) with the new settings inlib/compat/wordpress-6.0/class-wp-theme-json-gutenberg.php. This suffices to make the plugin work in WordPress 5.8. See this commit for example e2a1890 - Make sure the plugin calls the plugin classes (
WP_Theme_JSON_Resolver_GutenbergandWP_Theme_JSON_Gutenberg). Because our main interfaces are thewp_get_global_*functions, the changes in step 1 are not enough for WordPress 5.9 because those functions are part of core and call the core classes. In WordPress 5.8 it works fine because it uses the plugin classes. How can we override the functions in 5.9? We can't, so we instead call themgutenberg_get_global_*in the plugin and use those in all the instances. This commit renameswp_get_global_settingstogutenberg_get_global_settingsand changes all the instances cf100a2
That's it. Note that we only need the second step once: when this is done, subsequent PRs won't have to do it, they'll only require the changes in step 1.
Andrew, I've noticed you have added layout.position to the properties metadata, so I understood that'd be a valid style going forward. I went ahead I repeated the process I did with settings:
There was a problem hiding this comment.
I've figured there are a few in-flight PRs you all are working on and we don't know which will land first. So I've gone ahead and extracted the changes done in step 2 (both settings & styles) to a standalone PR so we can fast-track them and your PRs focus on feature work #38883
Once that lands, this PR can be rebased and the related commits removed.
There was a problem hiding this comment.
cc @andrewserong @ramonjd @aaronrobertshaw @ajlende @scruffian for awareness of this thread, as I know you all have been working on this area.
There was a problem hiding this comment.
Huge kudos for helping on this @oandregal! 🙇 And for the great explanations. It's a massive help. Thank you!!
There was a problem hiding this comment.
That's fantastic, thanks so much for jumping in and committing to the PR @oandregal, much appreciated! We'll take a look at #38883 today 😀
ramonjd
reviewed
Feb 17, 2022
Rename wp_get_global_settings to gutenberg_get_global_settings. In WordPress 5.9 the wp_* function is already defined, so we can't override them. It's calling the existing WP_Theme_JSON classes in WordPress core so it won't pick up the plugin modification. In the plugin, to make sure these changes work fine in 5.9 as well, we need to use the gutenberg_* function instead.
…e Gutenberg classes
oandregal
force-pushed
the
try/add-sticky-position-support-to-group-block
branch
from
2ff4ed8
to
ca41acf
Compare
This was referenced Feb 17, 2022
ramonjd
reviewed
Feb 18, 2022
| 'custom' => null, | ||
| 'layout' => array( | ||
| 'contentSize' => null, | ||
| 'position' => null, |
There was a problem hiding this comment.
Does this need to be added to schemas/json/theme.json as well?
Asking for me :)
E.g.,
"settingsPropertiesLayout": {
"properties": {
"layout": {
"description": "Settings related to layout.",
"type": "object",
"properties": {
"position": {
"description": "Determines the sticky position of a block.",
"type": "string"
},
"contentSize": {
"description": "Sets the max-width of the content.",
"type": "string"
},
"wideSize": {
"description": "Sets the max-width of wide (`.alignwide`) content.",
"type": "string"
}
},
"additionalProperties": false
}
}
},There was a problem hiding this comment.
Yes, I'm pretty sure it will be! There's still a few other things I wanted to explore in this PR before adding that (like where to put top, right, bottom, left in the object, and whether we also need to store a zIndex value so that the sticky position will work — even if we keep that setting hidden from the user in the UI)
30 tasks
|
Update after a very long time away from this PR — the original implementation here was imagining storing the I'd love to have another go at this from the perspective of the current state of layout. I think some good things to try would be:
|
|
Closing out this experiment now, but let's follow-up separately with new explorations into how to achieve this feature. |
8 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.
Description
The goal is to solve #30121
🚧 🚧 🚧 🚧 🚧 🚧
This is a WIP exploring adding Position support to allow a group block to be sticky positioned, details TBC.
How has this been tested?
TBC
Screenshots
Work in progress:
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).