Sticky Position: Add a "Make sticky" action to the Template Part block #49085
Conversation
|
Size Change: +342 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 9d9a849. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4422991678
|
andrewserong
changed the title
andrewserong
added
[Feature] Design Tools
|
Thanks for the PR, exciting! It seems to work well, but I miss some kind of visual feedback/confirmation. This is where I think the Sticky Group variation we've discussed before would be really handy. Even if the Group name just became 'Sticky Group', 'Group (sticky)', or similar, that would help. Maybe it doesn't need to be a variation? |
|
Yep indeed — should we have a sticky icon next to the title, when stickied? |
|
Very cool! From what I noticed in the above video. |
There was a problem hiding this comment.
This feels natural to me, and worked exactly as described. Nice work!
I would echo @jameskoster's comments above, it could use some sort of indication that it worked otherwise you don't know until you try and scroll. I agree changing the name/label of the template part group block in the list view and on the canvas would be sufficiently indicative. 👍
|
Thanks for the feedback, folks! Totally agree that it'd be good to make it clearer that the wrapping Group block is sticky. I've had a go at this over in #49122 — it'll likely need some design feedback to figure out the right presentation for it. I thought creating more granular PRs could be helpful, so that we can iterate on each of the proposed changes in isolation 🙂
At this stage, I don't think we need to create a separate variation for the Group block — I think we can get a long way via labelling based on position values, and it will also help ensure that the position support is decoupled from the Group block, which should help for if/when we want to add position support to other blocks in the future.
It's also available in the settings menu from the block toolbar within the editor canvas, so the user doesn't need to have the list view open to find this action. It'd also be possible to add an action to the right-hand sidebar, but for now, I think the two sets of block settings menus might be a good fit — it ensures that the action is available, while not making it too prominent. But, happy for feedback on that idea of course! |
There was a problem hiding this comment.
Code looks good and it's working very well! This is so much easier than hunting around in the sidebar for the relevant control, I almost wish all root blocks could have it (though of course it wouldn't make sense to wrap any block that's not a template part in a Group)
Agree with handling the list view label separately, and
help ensure that the position support is decoupled from the Group block, which should help for if/when we want to add position support to other blocks in the future
can't wait for sticky Headings and Tables of Contents 😼
| if ( | ||
| ! isGroupable || | ||
| ! canRemove || | ||
| ! groupingBlockName || |
There was a problem hiding this comment.
Til about the concept of a "grouping block"! Does this mean blocks could be grouped with a different container than a Group block?
There was a problem hiding this comment.
Theoretically yes, but in practice the groupingBlockName is set up as the Group block when the core blocks are registered here. My understanding is that it's good when dealing with grouping (verb) behaviour to use this abstraction so that (in theory) custom editors that don't use core blocks can still work properly. In practice, I'm not sure how frequently that use case is realised, but for this PR, I thought I'd try to be consistent with the existing grouping logic!
|
Yay, thanks for the review @tellthemachines! I'll hold off on merging to wait for designers to let us know if they'd prefer we have the sticky labelling in list view present before landing this one. Good to know this is in decent shape for when we're happy with the labelling situation 😀 |
|
Since #49122 is already on the way I don't think this PR needs to be blocked. |
|
Thanks for confirming @jameskoster! With the approving review, I'll merge this in now. Happy to do any follow up if anyone runs into any issues. |
|
Thanks for all the work here. Coming back to this one, in my haste I think I missed the nuance where this wraps the template part in a group. Apologies, this is on me. But essentially I'm not sure it's the best experience after all, it isn't obvious how to undo this action. But perhaps more importantly, I always saw the sticky property as something we eventually expand to more blocks than the group. In fact the main reason we started with limiting sticky to only the group (correct me if I'm misremembering) wasn't because that was going to remain that way forever, but to limit the surface for a somewhat advanced feature we weren't sure of. I always expected that as we found out how the feature got used, that it would expand to other blocks, like template parts, and simply be available there. In the case of sticky, it seems to have already proven itself hugely valuable with themes, so it seems like instead of adding this group wrapping functionality, we might just add it to the template part itself? No real harm done here if we need to revisit: there really is no danger in having a group block wrap your template part. But I think it might still be worth it to revisit this flow. What do you think? |
|
Applying position directly to the template part always results in a question of whether that property is synced or not. Seeing as every other property is synced, you would likely expect position to be synced as well. But that doesn't feel clear cut to me. You might want the header to be sticky in some templates, but not in others. If we did want to allow this to by synced globally, then we'd need to come up with a way to override it locally. I've always come to the conclusion that ultimately it's the responsibility of the parent document to position its constituent blocks. And that we can/should do a better job of communicating the sticky regions in the UI (#49122). |
|
I like 49122, especially if we go an icon-route. Synced globally or locally is a valid conversation to have, one that I do recall coming up for TPs before. But is that solved well with the grouping container? You could put a group inside the template part, make that sticky, if you wanted it synced. I know, there's a bit of a devils advocate tinge to that statement, I don't mean to dismiss your feedback. The thing is, if we do mean to eventually roll out sticky positioning to virtually all blocks, it feels weird that there's a dedicated "make a group and apply sticky" feature just for the template part. |
|
I do agree in general, but without a clearer solution for the global / local issue it seems necessary to me. What we need is a way to override template part / reusable block properties in a local context. Similar to how you can apply overrides to component instances in Figma. Then you could say the Header is sticky by default, but in this one template I am going to override that and make it fixed, or static, or whatever. I guess this has some overlap with Styles, where you might want to style elements such as links in the Paragraph block at a global level, but override those styles in certain contexts. cc @SaxonF.
You can try, but it probably wouldn't work as expected due to the |
|
I wouldn't oppose a more global solution. But as far as local, I don't think we should go with this "add a group and set it to sticky" in the mean time. It seems a bit random and temporary, and it makes the menu all the longer and even inconsistent. Again, sorry I didn't catch this sooner. |
|
The only short term alternative I can think of would be to add the Position panel to the Template Part block:
But we'd need to decide if it's applied locally or globally, and communicate that to the user. For some added context we used to have an option to change the background color here, and folks got confused when they added a background color and found that it wasn't applied globally. For that reason I'd lean towards making it a global change. But the trade-off there is that you wouldn't be able to make a template part sticky in some templates and not in others. Did you have any other ideas? 🤔 |
|
That was my suggestion. This would be applied locally, right? That way, nothing to communicate since it would always be local here, just like anything that's in the Advanced section. And you'd be able to mix and match. |
|
Thanks for the feedback here folks, just to make sure we're on the same page, would you like me to revert this change? If you'll indulge me, I'd like to advocate for keeping this change, but happy to disagree and commit if you confirm you'd like it removed for now 🙂 Here's some more context on how we arrived at the current state in this PR, and some responses to the discussion so far:
Unfortunately figuring out how to apply styles across all instances of a template part block will be non-trivial. In #47133 where we explored adding the Position panel to the template part block locally, the block attributes are stored in the markup of the parent template and not within the template part itself. We currently don't have a concept of block attributes being stored at the root level of the template part itself, and we would need to figure out where in the database those block attributes would be stored — so I believe figuring out root template part styles that are applied globally will wind up becoming a bit on an API problem. I'm keen to avoid that complexity for now while we come up with pragmatic UI improvements that can ease the UX problems with interacting with the position support. In the longer-term, I think exploring the globally styled template part block idea is worth pursuing, but since it'll involve API changes, and working out where the block attributes will be serialized, it sounds like an area for more design exploration and technical research rather than something we can immediately jump in and try implementing. To try to phrase this differently: If we accept that in the short-term (and say, potentially for the next major WP release) we will not have the ability to set sticky on the template part block itself, and have it apply to all instances of that template part, what things can we do in the UI to make it easier for folks to create sticky headers? Again, thanks for all the feedback! Happy to put up a revert PR if that's preferred, and have another go at improving the UX here in subsequent PRs 🙂 |
I agree with @jameskoster suggestion here, but sticky position should only apply locally. We should think about the future of patterns where a pattern could be synced across your site, possibly replacing template parts and re-usable blocks. In that future, you should be able to apply/override certain local attributes of patterns. For example, setting the width or changing the colour set of a local instance of a pattern. You shouldn't have to wrap a pattern/template part in a group for every local change. To change something globally , regardless of what it is, I think we need to start establishing a common pattern so people feel confident in the context they are editing in. For example, a dedicated "edit" button that enters focus mode or changes toolbar title (my page -> my header) |
I agree. #43608 is relevant to that discussion. |
I don't think we should keep this in. Wrapping the template part in a group for you, and applying position to that group, are the right steps to do this currently, but it's not the right experience we should be optimizing for. I think it'll confuse folks, more than help. It makes it seem like you can apply the position to the template part, but you really can't. Also, the template part will then be nested, which can further confuse folks — as you loose context of where that template part is now. This would also introduce inconsistencies on how sticky is applied, when opened up to other blocks perhaps. We don't have design tooling in the block toolbar more menu elsewhere—and I don't think we should perhaps. Those menus are more managerial (copy/duplicate/delete/lock). |
|
Thanks for all the discussion, feedback and clarity here! I've opened a revert PR over in #49219 Happy to have another go at improving the UX here once we've got a better sense of the next steps we'd like to take. |
What?
Part of #47043
Add a "Make sticky" action to template part blocks, that wraps a Template Part block in a Group block with sticky position settings set. This action is visible in the block settings menu, either in the editor canvas, or in the list view. It is only visible if the Template Part block is at the root level of the document.
This explores an idea from this comment: #47133 (comment)
Why?
To make it easier for a user to set a Template Part block to be sticky — rather than having to know to create a Group block and wrap the Template Part block with it, hopefully this action should be a little easier.
How?
Use a similar approach to the Grouping buttons/actions:
0pxto stick the block to the top of the viewport when scrolled past the edge of the screen.Testing Instructions
appearanceToolsset totrue.0.theme.jsonfile to setsettings.appearanceToolstofalseand reload the site editor. When the Header template part block is at the root of the document, you should not have a "Make sticky" option available in the settings menus.Screenshots or screencast
The flow for wrapping in a sticky group — note that the Ungroup menu item is a simple way to revert if folks need it:
Make-sticky-button.mp4