Add icons to block settings dropdown #34785
Conversation
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
youknowriad,
dcalhoun,
mtias,
gziolo,
ciampo and
jasmussen
ntsekouras
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
|
Size Change: +1.44 kB (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
|
Thanks for the PR, and thanks for the screenshots. Looking at these, moving the icons back to the left again feels like it topples some of the balance, and makes dropdown menus artificially wider due to the reserved space for icons. In these menus the icons work on the left because they are in separate or dedicated groups:
Following that pattern, we could rearrange to put insert before/after as part of a "tools" section of the menu, like so:
Even if we have to reserve space, it seems possible to have an overall narrower menu still:
Separate from this PR, but wanted to briefly resurface the "Show/hide more settings" menu item. If I recall correctly, it was created initially to provide an avenue to navigate from the block to the inspector. If that's right, it doesn't seem to be working as intended anymore, and it might be moot due to a change in tabbing behavior: press tab from any block to go to the inspector, press shift tab to go back. You navigate between blocks using the arrow keys. What remains is an inspector toggle in an odd place. Should we consider removing that menu item, and instead showing the keyboard shortcut as an inspector toggle tooltip?
|
| @@ -79,7 +84,7 @@ export function MenuItem( | |||
| className="components-menu-item__shortcut" | |||
| shortcut={ shortcut } | |||
| /> | |||
| { icon && <Icon icon={ icon } /> } | |||
| { icon && iconPosition === 'right' && <Icon icon={ icon } /> } | |||
There was a problem hiding this comment.
(same as #34710 (comment), since this PR includes the same changes to MenuItem)
This way of adding an icon to the right of the Button's content (as opposed to the left) feels hacky — have you considered adding this functionality directly on the Button's component?
There was a problem hiding this comment.
After discussing this a bit more with @ciampo in dm and exploring the code a bit, it seems that we have been hacking our way around for many current controls. It seems that last year I had added iconPosition in Button but used it with text added here.
Tool Selector uses a similar 'hack' by passing the icon as a label along with text.
So these usages and others that will surface by searching need to be consolidated through a more consistent API. We will probably need to make MenuItem always use the icon as a prop and also pass the iconPosition. This requires a greater effort and being extra cautious not to brake existing UIs and should be addressed in a separate PR.
ntsekouras
force-pushed
the
add/icons-to-block-settings-dropdown
branch
from
d4b5804
to
c161911
Compare
I pushed a change to narrow the specific menu with the following rationale..
I'm not sure if it would make sense semantically to move this options to a different group than they already are, but of course not having a strong opinion, as is more of UX decision.. In addition it's possible for a third party to extend this settings using PluginBlockSettingsMenuItem. If they add items with no |
|
Thanks for testing @dcalhoun! Does anyone think there is something more to be done in this PR? Should we move forward with it? |
|
Unifying menu items so they are simple components that are reused across menus with zero bespoke hacks is an important high level goal. Since it seems we have a need for icons on the left in terms of tools/alignment menus, it's possible we'll want to move them back there in the name of unification. However moving them to the right was a key aspect of #18667, so I'm a little hesitant to make that change as part of adding new icons to the block settings dropdown, without a little more design thought than I was able to give it. CC: @pablohoneyhoney @mtias do you have strong feelings here? |
|
Thanks for the ping @jasmussen It makes sense to me that we keep iterating on these component that has evolved with #18667 but hasn't in a while and should be challenged routinely. Dropdowns have allowed the toolbar to grow in functionality without feeling excessive while feeling like an extension to the toolbar rather than a foreign element. It was also highlighted that the use of iconography within dropdowns was spurious and unnecessary. I do think that forcing icons in all dropdowns doesn't account fully for contextuality where the icon might be more of a distraction that a service to secondary actions themselves or even focus on shortcuts. If we are looking to unify these components to have the icon in a common position, then left icons could make sense, but again I would not force all dropdowns or even all actions hosted in dropdowns to have an icon and make that judgement by case.
There are counted cases like the above one that can be easily corrected to be on the left, or have a completely different approach to signal external link, like:
I do see more urgent to rethink the order of the actions in this block settings dropdown, which will provide a better taxonomy but at the same time challenge some of the mock ups above since elements with apparent need of icons will be placed sparsely, likely not ideal. A better taxonomy could be to order or associate (if grouped and separated):
And then decide which actions do deserve or need icons, without pursuing a noisy approach where icons don't represent well the actions (and are hard to create in the first place), generate a convoluted UI that is more distracting than clear, or even affect how the no-icon actions and full dropdowns look. |
Description
Resolves: #25274
This PR adds some icons to some block settings at the toolbar dropdown to give more prominence to them. The changed options are per @mtias #25274 (comment).
This PR has some changes at
MenuItemfrom this PR, which is not merged (at the time of this PR's creation).I have also updated
PluginBlockSettingsMenuItemwhich is used forBlockSettingsMenuControlsslot. You can use the example here to check that everything seems to work fine with added items.Testing instructions
moresettings of a block's toolbarScreenshots
Before
After
Tasks
native/mobile--cc @dcalhoun @guaraniChecklist:
*.native.jsfiles for terms that need renaming or removal).