Position Panel: Open by default if a position type is set #49151
Conversation
andrewserong
added
[Type] Enhancement
andrewserong
requested review from
apeatling,
tellthemachines,
aaronrobertshaw and
a team
|
@aaronrobertshaw I've added you as a reviewer for this one since you've done a lot of work on the tab panels, and just in case there are any concerns with passing down the |
andrewserong
changed the title
|
Size Change: +787 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4da0531. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4474679256
|
|
I don't have a strong opinion for or against this small QOL change, but I do think that there might still be a chance that the entire position UI moves or changes a bit in the future, whether to live bundled with some other tools, or otherwise be rephrased or tweaked. That doesn't mean we shouldn't land this one — it seems fine to have in the interim. |
There was a problem hiding this comment.
Thanks for the ping @andrewserong, this looks like it would be a nice little improvement 👍
Following the test instructions, it works well, though there could be an edge case that might warrant revising the current approach.
As this PR stands now, it will only open the position panel when there is a single block selection with a position value set. I'd expect the same behaviour when selecting multiple blocks that have a position set.
Given we're already leveraging a block editor store selector to get the block attributes, perhaps we could lean into those selectors and avoid the need to plumb the clientId down through the tabs.
We could use getSelectedBlock if we don't want to open the panel by default for multi-block selections, or we could use getSelectedBlockClientIds and getBlockAttributes to cover the multi-block side of it.
What do you think?
Screen.Recording.2023-03-17.at.5.11.42.pm.mp4
|
If this makes sense for one panel, would it make sense for all? Should we consider this more broadly? |
Yes it probably does make sense for all. I'd still prefer to slowly expand that range rather than just go all-in — it does, after all, add weight to the inspector, and the inspector interface might need some separate love. I.e. I'm not sure it's that valuable yet on the paragraph. |
Agreed.
Probably, though with the shifting towards the tools UI it's less of an issue imo. |
…irst id for attributes check
|
Thanks for the feedback and review @aaronrobertshaw! I've updated this PR to remove the prop drilling, and use a selector to grab the selected clientIds. To avoid having to iterate over all selected clientIds, I've just used the first of the selection to determine whether to open the panel by default — I think this is vaguely consistent with what's currently happening with the Color panel, but my thinking was that it might be better to keep the logic simpler if we can. I've also split out the Happy to keep tweaking if you think we should be a bit more thorough about the block attributes!
Good point, and thanks for taking a look @jasmussen! Absolutely agree, for context with some of these Position PRs I'm opening at the moment — my main interest in the short-term is to smooth some of the rough edges with the current implementation while we continue to explore our longer-term options. This way Very happy to dive in and have a go at an improved panel / integrating the controls elsewhere if and when we have a solid direction in mind! Happy to continue the discussion about the panel over in #46032 if and when folks have time.
Good question @jameskoster, as far as I can tell, the only panels remaining using the accordion style panel are Layout, Position, and Advanced. Layout is already open by default, and we ran into some blockers switching to the ToolsPanel as the overall layout controls needed a rethink. But, any of the panels using the ToolsPanel effectively use the approach of displaying values if they're set, which I think means this PR has a pretty consistent feel. If we like the behaviour in this PR, it might be worth having a go at implementing something similar in a follow-up for the Advanced panel. |
|
Thanks for iterating on this one 👍
Personally, I don't think we need to avoid iterating over a small number of selected blocks.
The color panel is a ToolsPanel, so there's no explicit decision to expand the panel or not. Given the color controls are generally default controls, that forces their display. In terms of the value displayed in those color controls, I believe you are correct, it is the first block's attributes used. That said, I think that is a different scenario than what we are attempting to determine here. As for keeping the logic simpler, I don't think the alternative is that complex.
Here's a diff that would update this PR to open the position if any selected block has a position set. It follows the color example and still uses the first block's value in the control. Example diffdiff --git a/packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js b/packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js
index 0150d4819a..a743e6c429 100644
--- a/packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js
+++ b/packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js
@@ -16,19 +16,23 @@ import { default as InspectorControls } from '../inspector-controls';
import { store as blockEditorStore } from '../../store';
const PositionControlsPanel = () => {
- const { blockAttributes } = useSelect( ( select ) => {
- const { getBlockAttributes, getSelectedBlockClientIds } =
+ // Determine whether the panel should be expanded.
+ const { initialOpen } = useSelect( ( select ) => {
+ const { getBlocksByClientId, getSelectedBlockClientIds } =
select( blockEditorStore );
+
+ // If any selected block has a position set, open the panel by default.
+ // The first block's value will still be used within the control though.
const clientIds = getSelectedBlockClientIds();
+ const multiSelectedBlocks = getBlocksByClientId( clientIds );
+
return {
- blockAttributes: getBlockAttributes( clientIds[ 0 ] ),
+ initialOpen: multiSelectedBlocks.some(
+ ( { attributes } ) => !! attributes?.style?.position?.type
+ ),
};
}, [] );
- // If a position type is set, open the panel by default.
- // In a multi-selection, use the first block's attributes for the check.
- const initialOpen = !! blockAttributes?.style?.position?.type;
-
return (
<PanelBody
className="block-editor-block-inspector__position"
This split looks good, I think it also offsets any real issue in looping a few selected blocks. |
…k has a position type set. Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
|
Excellent, thanks for the diff and the collaboration on this one, @aaronrobertshaw, appreciate it! 🙇
Yes, looking over the diff and the way you've written it, I'm not concerned about the iteration now. This is testing well for me, and looks much cleaner. I've committed the change 👍 |
There was a problem hiding this comment.
Thanks for iterating so quickly @andrewserong 🚀
I thought we were nearly ready to land this but I noticed another quirk we might need to prevent before we do. Apologies in advance for the hassle.
When a block has sticky set as its position, the position panel is in fact opened by default. If you then switch that existing sticky selection to default, the panel will close. This doesn't happen on trunk because you need to interact with the panel to open it so that state is maintained. I rolled back the last commit as well and it appears to still be an issue there too.
| Trunk | This PR |
|---|---|
Screen.Recording.2023-03-21.at.10.20.23.am.mp4 |
Screen.Recording.2023-03-21.at.10.21.19.am.mp4 |
|
Great catch, thanks for the thorough testing! Yes, it looks like the logic is being re-evaluated once the block attributes are changed. I'll see if we can maybe add a |
|
Update: in ffc76f7 I've had a go at using a Happy to keep iterating/tweaking if there's a better way to do it! |
|
One caveat of the current approach is that there might be a slight flicker when selecting a block that has a value set, as it'll render with |
|
Update: switched to |
There was a problem hiding this comment.
I appreciate the effort in fine-tuning this one @andrewserong 🙇
While testing again I found another small edge case however I don't think it is a blocker if you'd like to address it in a follow-up, if it is even worth that.
With this PR's changes applied, if you select 2 blocks without a position set between them, then you get the collapsed position panel. So far so good. If you select multiple blocks where one has a position set you'll get the expanded panel. Also, good.
If you select multiple blocks without a position set between them and then add a block to the multi-block selection that has a position set, the panel will still be closed. A little unexpected perhaps 🤷
Again, I'm not even sure this is worth addressing or if it is expected behaviour.
Screen.Recording.2023-03-21.at.5.58.37.pm.mp4
All in all, I'm happy to approve this one and leave the decision on follow-ups to you 👍
|
One thing the video above exposes is an issue around the display of panels and their contents during multi-select.
In this circumstance I'd expect to see something in the panel that tells me the values are mixed, as well as a way to apply a new value to both. I guess it's worth a separate ticket, sorry for the detour! |
|
Thanks again for the detailed testing @aaronrobertshaw!
Great catch. I think I'll leave this edge case for now — I really like James' idea of communicating when values are mixed, so I'll leave looking at this to a follow-up, as there could be a good opportunity to work on both those ideas at the same time. For now, it'll be good to have this small UX improvement in, I think. Thanks again for the reviews! 🙇 |
andrewserong
deleted the
try/set-position-panel-to-open-if-a-value-is-set
branch
What?
Part of #47043
Set the Position Panel in the block inspector to open by default if a value is set for the position type.
Why?
As raised in reviews of #49085, it'd be good to improve the visibility of "sticky" in the UI when the value is set. One approach for the list view is being explored over in #49122, however I thought it'd be good to improve the visibility within the sidebar inspector, too. While usually we like to deprioritise Position by keeping the panel closed by default, if a value is set, I think it'd make sense to have the panel exposed by default — a little bit like how ToolsPanelItems are displayed if a value is set.
It's a fairly subtle change, but I think will improve visibility as users are clicking around between blocks in the editor.
How?
clientIdto the settings tab in the block inspector (it's already being passed down to the styles tab)clientIddown to thePositionControlscomponent (the position controls panel)Testing Instructions
Screenshots or screencast
The following screengrab demonstrates clicking between a Group block with no position set, and a Group block with Sticky position set. Observe the state of the Position panel at the right-hand side, and its default state when the Group block is selected:
2023-03-17.14.58.09.mp4