ToolsPanel: Refine component behaviour #34530
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
changed the base branch from
trunk
to
try/inspector-controls-with-toolspanel
|
Size Change: +444 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
aaronrobertshaw
changed the title
aaronrobertshaw
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
It's worth noting here that the current iteration actually sets Open to other ideas or opinions though. |
|
Thanks for tackling this one! I had a bit of a challenge getting things to work as your recording; my Cover block still has a separate min-height panel, and I couldn't get margin to show despite trying a few different themes, and updating the Group block.json file. However I did see this:
Some good steps here: the default panel is separate, I can reset it using the minus, I can reset all, and the check clears and removes non-default options. We need the new icons from the mockup, these:
Updated New The new check is actually a bit smaller than the existing one:
I see that we already have a minus, I had forgotten that. Do you know if/where it's used at the moment? We might want to rename it to "reset" and replace it, depending on where and how much it's used. I'm not sure whether we want a new Let me know if you'd like me to add these separately. |
There was a problem hiding this comment.
Thank you for starting to work on this, @aaronrobertshaw !
I've left a few initial inline comments, and I'm going to follow the progress of this PR in the upcoming days
packages/components/src/tools-panel/tools-panel-header/component.js
Outdated
Show resolved
Hide resolved
| } | ||
| ) } | ||
| </MenuGroup> | ||
| { !! defaultItems?.length && ( |
There was a problem hiding this comment.
This block is starting to get quite hard to parse, have you considered extracting it to a separate component? (it could live in this same file and it wouldn't need to be exported).
Also, have you considered using ternaries instead of the && operator?
There was a problem hiding this comment.
This block is starting to get quite hard to parse, have you considered extracting it to a separate component?
Agreed. I've wavered on extracting the MenuItems to their own components previously but it didn't reduce the code much. I'll look at refactoring the menu groups out into separate components instead.
Also, have you considered using ternaries instead of the && operator?
In this instance, the use of !! was to specifically guard against the issue highlighted by that link. The && operator still appears to be the prevailing style within the codebase. I understand that using a ternary makes missing the !! a non-issue. Is this something that should be added to the Gutenberg coding guidelines?
There was a problem hiding this comment.
I'll look at refactoring the menu groups out into separate components instead.
Sounds good! Thank you :)
In this instance, the use of !! was to specifically guard against the issue highlighted by that link. The && operator still appears to be the prevailing style within the codebase. I understand that using a ternary makes missing the !! a non-issue. Is this something that should be added to the Gutenberg coding guidelines?
Thank you for the explanation about using !!. I'm not sure if this should be added to the coding guidelines or even as an ESLint rule — that is something that @gziolo may advise about!
packages/components/src/tools-panel/tools-panel-header/component.js
Outdated
Show resolved
Hide resolved
Just wanted to flag that I also had the same issues when trying to follow the instructions in the PR. I also tried to use Storybook, but visiting the ToolsPanel story at the moment generates an error:
|
|
Thanks @jasmussen for your time and effort as always 👍
The theme I was testing with already had the To test with the cover block's min-height control injected via the proposed inspector controls slots, you'd need to apply commits from #34065 on top of this PR. I did this to test the updated ToolsPanel with a SlotFill injected control, I didn't intend for you to need to do it at this point in time. I fixed the issue with the ToolsPanel story that @ciampo highlighted so that is the easiest option for getting your hands on the updated panel now.
Awesome, thank you. I've added the new icons.
At a quick glance, it only appears that the minus icon is used for the iOS stepper.
I added the icon as a new check icon rather than replace the existing one. |
|
I'm happy to update the check and reset icons however needed. Including handling them via a separate PR if the extra changes to existing icons are preferred. |
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
f058696
to
574c27b
Compare
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
574c27b
to
e9bbbed
Compare
aaronrobertshaw
force-pushed
the
try/updated-tools-panel-ux
branch
from
79f9a93
to
091b5af
Compare
aaronrobertshaw
changed the base branch from
try/inspector-controls-with-toolspanel
to
update/tools-panel-for-slotfills-use
There was a problem hiding this comment.
From a quick look, code changes LGTM! I also tried the component in Storybook and it seemed to work as intended.
I'll let @jasmussen give the final seal of approval ✅
There was a problem hiding this comment.
This is definitely an improvement, and should be merged.
I do want to note that I think there's still too much assumed knowledge wrapped up in how this works. For example this first experience for someone who has never seen this before:
I don't think it's clear what the difference in expected action between the minus and check icon is. It's assuming I know that the padding is a default control and therefore can't be "unchecked".
It's a tough one when only icons are being used with no labels indicating which controls are the default and therefore can't be removed. I don't have any good suggestions, but wanted to note this as something to think about further.
Default and optional controls will now be displayed via separate menu groups and behave slightly differently. Default controls will always be shown and display a checkmark when empty or a minus icon when customized.
Adds default controls so that the updated behaviour separating default and optional controls into menu groups can be seen via the story.
Instead of adding a second smaller check icon, we are replacing the existing check icon as it is not widely used.
aaronrobertshaw
force-pushed
the
try/updated-tools-panel-ux
branch
from
72cf4a7
to
622206a
Compare
|
✅ The |
I agree with this. Solving it is a tricky one though.
Adding some tooltips to the panel's menu items was trialled but it made the UI incredibly busy, undermining any improvements to the UX.
I'd be happy for this to land once @jasmussen has a chance to approve the PR in its current state. We can then iterate on this further in follow up PRs. Merging this will also allow for the |
|
Took it for a quick spin, and the paddings, margins, and behavior looks right to me:
The goal of the interface is to provide additional tools first and foremost, and secondarily to provide an additional and intentionally de-emphasized affordance for clearing values. Knowing that this interface can and will be iterated upon in the future, it feels important to start as simple as possible. In that vein, let's definitely ship this one. And if we find the need to adjust further, we will. Thanks Aaron! |
Fixes: #34257
Depends on:
Description
ToolsPanelto improve its UX following the suggestions outlined on the corresponding issue.ToolsPaneldropdown menuHow has this been tested?
Manually:
Test instructions
settings.spacing.customMarginproperty totruein your theme.json file.Steppercomponent displays its check icon correctly. AnyUnitControlon native will show thisSteppercomponent, one easy block to test with on native is the Cover block.Screenshots
Cover Block with cherry-picked commit from #34065
Screen.Recording.2021-09-06.at.4.50.05.pm.mp4
Storybook
Screen.Recording.2021-09-08.at.12.00.56.am.mp4
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).