Sticky Position: Try re-enabling non-root sticky position #49321
Conversation
andrewserong
added
[Type] Experimental
|
Size Change: +45 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
andrewserong
force-pushed
the
try/add-position-visualizer
branch
from
fa17664 to
ae867f6
Compare
| } | ||
|
|
||
| return ( | ||
| <BlockPopover |
There was a problem hiding this comment.
Just a note: To fix the issue where the popover goes off the edge of the screen, it might be worth experimenting with Popover instead and then pass it a rect (or getBoundingClientRect() function) so that we can ensure it's only rendering over the visible area of the page. I.e. treat this a little bit like the drop indicators.
There was a problem hiding this comment.
Potential fix for this issue in #49978
andrewserong
force-pushed
the
try/add-position-visualizer
branch
from
ae867f6 to
fbe05ae
Compare
andrewserong
force-pushed
the
try/add-position-visualizer
branch
from
fbe05ae to
27dc262
Compare
|
Update:
|
|
In f2f4682, I've fixed up the dimensions of the BlockPopover so that when the browser window is resized (or when scrollbars appear) then it gets recalculated. If it turns out that this shouldn't be behaviour for all BlockPopovers, then we can always move the logic into the PositionVisualizer instead. For now, I'll leave it where it is, and we can look at it in more detail once this PR is ready for review. Edit: it could possibly be better to use a |
|
Flaky tests detected in b9e7b37. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5285159526
|
andrewserong
force-pushed
the
try/add-position-visualizer
branch
from
f2f4682 to
9e42a8b
Compare
|
This is getting closer to a testable state, but one issue I ran into is that the 2023-04-26.15.02.53.mp4In a long post that contains sticky headings, this behaviour of sitting on top of the other sticky element could actually be desirable. What I'm wonder now, is whether we might want to give the user the ability to set a top offset value for the sticky position? |
andrewserong
requested review from
jameskoster,
jasmussen,
apeatling,
ramonjd and
tellthemachines
|
Update: this PR should be ready for review now — it's an experimental idea to see if adding text, plus a visualizer for sticky positioning will help improve the UX for sticky positioning at non-root positions. Feedback and alternate ideas are very welcome! For example, we could do a lot of adjustment to how the sticky visualizer appears, tweak text, etc. I noticed one potential block, which is that if the site uses sticky position for its header, then the element that's set to sticky doesn't know that it should have a |
|
Thanks for considering this. It's an excellent instinct, the use case for non-root locations is valid, the video is helpful, and the example code even moreso. Here's a GIF just showing me experiencing the same as your demo:
The key take-away here is shown in this image:
The light blue border shown in the canvas here is meant to denote the parent container, so that it's clear what the block sticks to. This actually reminds me a fair bit of some similar explorations we've had in the past, in general around template parts, ancestors, etc, having explored dotted lines or similar. #44774 and #44775 are tangentially related. Because these block outlines have come up many times in the past, I'd appreciate input from @jameskoster or @SaxonF, because when to show outlines and what they should look like is something we need to get right. Is contextuality like suggested here the key? Should the outline be shown when the Sticky option is focused or interacted with, and ten fade out? Or should it show when the block is selected? Should it be shown when a block is hovered? Should it be a dotted line? Should it be something else, like dimming non-selected blocks, more like spotlight mode? |
|
I'm curious about the motivation to indicate the sticky block container on the canvas? It will always be the direct ancestor, so it doesn't seem super useful to me. I am likely missing something though. Overall I wonder if it would be better expressed in List View. I think we tried some explorations around that before but didn't land anything yet. |
|
Thanks for the feedback and suggestions @jasmussen and @jameskoster!
Good question — when we first landed the sticky position support, based on user feedback, it turned out that it was confusing for folks that when they set a non-root Group block to Sticky, they thought it'd stick to the top of the page, and the behaviour that it sticks to the direct ancestor wasn't clear. Based on that feedback, we removed the non-root sticky support until the UX could be improved (here's a wrap up comment from that time). So, the problem to solve is how do we make it clear to a user what's happening when they set a block to
We haven't tried anything that flags which parent it relates to yet, I don't think. There's an open PR (#49122) to flag the status of sticky, but not the relationship between the sticky block and its parent. While improving things in the list view is a good idea, my feeling is that we'll need something a little more prominent in the editor canvas in order for the feature not to feel broken for users that set sticky in nested blocks but don't really know what's going on.
These are exactly the kinds of questions I was hoping for! I think showing the context for the user is key, similar to how we have padding and margin visualizers to help users understand those concepts. Determining exactly when to show the visualizer, when to hide it, and what it should look like are all great things to explore. Let me know if anyone has ideas about how this might look, and I'm happy to continue hacking around 🙂 |
| }, | ||
| [ clientId ] | ||
| ); | ||
|
|
||
| const blockInformation = useBlockDisplayInformation( firstParentClientId ); | ||
| const stickyHelpText = | ||
| allowSticky && value === 'sticky' && blockInformation |
There was a problem hiding this comment.
Would it be worth checking against STICKY_OPTION.value?
There was a problem hiding this comment.
That'd be neater, yes — I'll update. Thanks for spotting it!
For the mobile menu, would it be dodgy to hide
This sounds like a reasonable and non-invasive first approach for now. How long term would this be? I'm thinking backwards compatibility when and if we add z-index to block supports. I suppose Having said all that, I've tested this given those constraints and I think it's in good shape. Stickyness in most cases works as I expect and the visualizer appears when I hover over the stick item in the drop down only. |
|
This is cool, this could work:
Can the blue indicator be below the sticky block? I.e. indicating only the parent? Not crucial, but would be a nice little nod. This one: 
The gray hover state for an item here is very dark and means the contrast isn't good. I imagine it's a separate issue with the dropdown control, just like the 2px focus state, but mentioning here just in case it's local. Would it be possible to also show the highlight if you hover the "Sticky" state in this dropdown? I.e. imagine it's set to default, and you hover the sticky state to show the parent container it'll stick to? Also not crucial at all, if it's hacky or even minimally hard to implement, not worth it, just wondering. I would love a check from @jameskoster but on my side, especially if we can put the highlight below the sticky block, I could get behind this! |
|
Thanks for taking this for another spin, Joen!
Yes, I think that issue is with the control, but it'd be a good one to improve.
Unfortunately the
Oh, good idea. I'll have a play with that when I'm back next week. One potential issue is that if there isn't much content yet, then hovering over the dropdown might not show anything if we set the visualizer to be underneath the sticky block. So even if it doesn't look very pretty, it might still be useful for it to be overlaid so that it's clear when the sticky region is very minimal? Just a thought, but I don't feel too strongly about it. The other challenge is that depending on how the popover works, it might be tricky to get it to display underneath 🤔... either way, I'll play with it and report back 🙂 |
|
In terms of functionality this is working well, but the visualiser doesn't quite hit the mark for me. It may be subjective, but this treatment doesn't communicate the behavior in an intuitive way. Is it vital? Could we explore this detail separately? |
|
Thanks for re-testing!
Unfortunately, I think having some form of visualisation is probably a requirement for reintroducing non-root sticky position blocks. Feedback when we initially introduced sticky position was that without having some form of visualisation, users unfamiliar with the CSS property would find the behaviour confusing that I'm happy to keep chipping away at this PR and/or try out other ideas. Also happy to merge in a reduced version of this PR that enables non-root sticky positioning + the help text, without the visualizer, if we can get consensus 🙂 |
|
Can we try adding some visualization to the list view when this is set? (An icon with a tooltip, perhaps, or a badge that says "sticky".) |
Yes, we started exploring this back in #49122 but wound up being blocked by some work that's now landed to improve the accessibility of how blocks are read out in the list view. That work has landed now, so it should be ready for a rebase. I'll dust it off tomorrow. |
|
Hey @andrewserong, this is looking great! Are we aiming for 6.3 with this? #47043 is on the 6.3 Project Board, so I wasn't sure. If so, I'll add this PR to the board as well. Thanks! |
Thanks for the ping! Yes, my hope is to land both this and #49122 as the main parts of the #47043 tracking issue for 6.3. It currently depends on whether we can reach a good balance in terms of design and functionality in time, but that is currently my aim. |
|
Great, thanks @andrewserong. I updated the board accordingly. |
andrewserong
changed the title
…visualizer to define sticky area
… select inner blocks
…f the block is selected
andrewserong
force-pushed
the
try/add-position-visualizer
branch
from
bbae9fa to
b9e7b37
Compare
|
Update: I have pared this PR back to just re-enabling non-root sticky positions, and the inclusion of the help text, and removed the visualizer for now. We also have #49122 ready for review, which adds the sticky icon to the list view. There is still an outstanding issue for sites that have sticky headers and also use sticky blocks elsewhere in the post content, as flagged by @tellthemachines earlier in #49321 (review) — that issue technically exists irrespective of this PR, so I would lean toward looking into fixing that separately. I believe the main way to fix that would be to either allow individual control over z-index values, or add some clever logic that adjusts the z-index for the sticky block based on whether or not it contains a template part. Either way, I think it's best to look into fixes separately if we can. I think this PR should be ready for a final review now. To recap, the PR now does the following:
Thanks again for the feedback, everyone! 🙇 |
|
Thanks for reviewing @ramonjd! Since we have clearer ideas now about which visualisation aspects we do or don't want (no the visualiser, yes to adding an icon in the list view), and that the other changes in this PR appeared to be non-controversial (adding in help text, re-enabling non-root positions), I'll merge this one in now. If there are any objections or issues over the weekend, I can spin up a revert on Monday — this PR only really changes whether or not the control is visible in non-root positions, so it'd be easy to return to the previous state, with plenty of time before the next Gutenberg RC. Thanks again for all the input, everyone! |
…49321) * Sticky Position: Try re-enabling non-root sticky position, and add a visualizer to define sticky area * Fix whitespace * Ensure the position visualizer does not interfere with the ability to select inner blocks * Ensure resizing the block editor will update the block popover dimensions * Try simplifying the display logic so it always shows the visualizer if the block is selected * Small code quality tweaks * Try dotted outline, subtle display on focus and hover label * Fix help text and spacing * Update visualizer to only display on hover * Remove visualizer
What?
Part of #47043, will ideally resolve #47892
This PR explores re-enabling sticky position at non-root positions within the document, and adds paragraph / help text beneath the position drop-down selection when Sticky is set, so that we describe in text that the block will be sticky to its parent.
Early versions of this PR also explored adding in a position visualizer of some kind, however it was uncertain as to how to make this truly beneficial or useful. For now, this PR has been pared back to just re-enabling non-root positions, and adding in the help text.
Why?
For the initial version of sticky positioning, support for sticky being set at non-root positions was removed because of the potential confusion for users. The goal with this PR is to explore what sorts of things we can do in the UI to help ease some of that confusion, and to determine whether or not it's enough to re-enable non-root position values.
How?
stickyis set.To-do items
Resizing the browser doesn't update the dimensions of the popoverhe block popover extends beyond the screen and creates an additional scrollbar (Fix in: Edit Post: Hide overflowing content in visual editor wrapper to prevent block popovers from creating scrollbars #49978)If you hover over the parent block and attempt to scroll, it doesn't let youThe border of the block popover renders over the top of the block toolbarDetermine when to show / hide the popover (shows while block is selected for now)The size calculation doesn't quite work in non-100% browser zoom levelsTesting Instructions
Copy / paste some test markup below (a sticky group in a column):
Block markup for a sticky group within a column in a columns block
Or, construct your own pattern:
Screenshots or screencast
Site frontend:
2023-04-26.16.20.34.mp4