Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design improvements to Image Block toolbar #26411

Closed
jasmussen opened this issue Oct 23, 2020 · 16 comments
Closed

Design improvements to Image Block toolbar #26411

jasmussen opened this issue Oct 23, 2020 · 16 comments
Labels
[Block] Image Affects the Image Block [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@jasmussen
Copy link
Contributor

The Image block toolbar is heavy on borders, to the point that the visual separation doesn't help:

Screenshot 2020-10-23 at 10 36 44

In the above example, alignment, link, and edit are all block level actions, and conceptually related. These could be in a group together:

96862717-22eb5580-1466-11eb-99fe-14a5b924df55


Similarly, the "Edit" state of the Image block toolbar is currently quite heavy:

Screenshot 2020-10-23 at 10 38 10

In this context there's no reason for you to see the transform/move controls, or even the alignments, because you are in a modal section of the toolbar. It could be collapsed down to this:

Screenshot 2020-10-23 at 10 39 39

@jasmussen jasmussen added [Type] Task Issues or PRs that have been broken down into an individual action to take [Block] Image Affects the Image Block labels Oct 23, 2020
@mtias
Copy link
Member

mtias commented Oct 23, 2020

I'd love to ensure we are doing this more generically for all blocks

@retrofox
Copy link
Contributor

retrofox commented Nov 4, 2020

Could we try to define what we need for a generic scope, in a single sentence? I'll try but no guarantee at all 😅

Be able to group toolbars in the same section.

It means we shouldn't re-implement the current toolbars but be able to put them in the same section, adjusting the box model to make look the same as the single toolbar-group, when it isn't collapsed:

image

Maybe it's doable adding a new wrapper component, which could get the API more confusing. I've been diving into the code and I see that we have many Toolbars components, some of them are entirely internal components.
But in case we can live with that, maybe we can add a <ToolbarsGroupSection /> or something like that. It won't be too much more than an element with a CSS class, at least in the first approach.
Saying this because maybe we'd like to create a context per section in case we want to propagate data between the toolbars (please ignore this part, too complex, and maybe it doesn't make sense).

Creating this <ToolbarsGroupSection /> could provide a new way to organize the toolbars slightly different for each block. A media settings toolbar for a Media&Text block looks similar but different for the cover block.

@retrofox
Copy link
Contributor

retrofox commented Nov 4, 2020

Screen Shot 2020-11-04 at 2 56 09 PM

That is something we did playing with the Full Height Aligment, just for testing puporses. Two toolbars grouped in the same section.

@jasmussen
Copy link
Contributor Author

Could we try to define what we need for a generic scope, in a single sentence? I'll try but no guarantee at all 😅

Sure I'll try:

The use of toolbar-groups to segment controls in the block toolbar, currently, is random. Instead, controls should be grouped in descending order according to this pattern: [block controls] [block level controls] [inline level controls] [more].

In #24898 there's a mockup, which although it's missing the drag handle, is still accurate to the structure:

Note in the above image that the spacing is different. A full button is only 48px wide if it's a single button inside the toolbar.

In the case of the full height button for cover, it is a block level control, therefore it should be in the same toolbar group as the alignments dropdown, which is also a block level control.

@retrofox
Copy link
Contributor

retrofox commented Nov 5, 2020

The use of toolbar-groups to segment controls in the block toolbar, currently, is random. Instead, controls should be grouped in descending order according to this pattern: [block controls] [block level controls] [inline level controls] [more].

Very nice 👍 . Thanks!

In #24898 there's a mockup, which although it's missing the drag handle, is still accurate to the structure:

<3 Very nice.

Note in the above image that the spacing is different. A full button is only 48px wide if it's a single button inside the toolbar.

In the case of the full height button for cover, it is a block level control, therefore it should be in the same toolbar group as the alignments dropdown, which is also a block level control.

Yeah... this is the trickiest part from my point of view. I do think there could be some cases where we'd like grouping toolbar groups. For instance, the Media&Text block:

Alignment Vertical Alignment
image image

Maybe I'm wrong, but let's suppose we'd like to put these two toolbars (group) in the same section. Right now, with the current implementation, we should combine them in the same group. But clearly, to me, they are not part of the same one. Because:

  • They mean different connotations.
  • The options can be combined.

There are some options that are naturally self excluding. A simpler case is left - middle - right alignment. It isn't possible to set two options at the same time. But there are other alignments combinations totally doable, for instance, full width and full height. We are not able to handle these two options in the same group, for many reasons (open another list, sorry)

  • The toolbar shows only one "selected" option, which generally is the icon in the sidebar.
  • Generally, the option is stored in a single var. For instance, the Alignments toolbar stores its value in the align attribute, a string that can take a set of possible values: left, middle, and so on. How can we store two alignments combination there? `

Please help me to get rid of the idea of grouping toolbars-group, slightly described in my previous comment. In short, it suggests adding a new organization level, to put their toolbars-group.

I see three organization-levels (it doesn't mean to the so far, beyond the structure of the components:

The toolbar:

[ toolbar ] 

Into the toolbar, there are toolbars group:

[ [ toolbar-group-1 ] | [ toolbar-group-2 ] ]

And each toolbar-group contains some buttons:

[ [ [ button-1-1 ] [ button-1-2 ] [ button-1-3 ] ] | [ [ button-2-1 ] [ button-2-2 ] [ button-2-3 ] ] ]

I suggest adding a new organization-level in between the navigation and the toolbars group destined to them in the same section. It would allow combining Alignment toolbars and Vertical Alignments if it's desired, for instance.

@jasmussen
Copy link
Contributor Author

Good thoughts here.

Maybe I'm wrong, but let's suppose we'd like to put these two toolbars (group) in the same section. Right now, with the current implementation, we should combine them in the same group. But clearly, to me, they are not part of the same one. > Because:

They mean different connotations.
The options can be combined.

Bold and italic can be combined, but they remain in the same group. To me, the separation is about hierarchy rather than behavior, and in grouping based on the former, general rules that apply to all blocks can be made.

But there are other alignments combinations totally doable, for instance, full width and full height. We are not able to handle these two options in the same group, for many reasons (open another list, sorry)

  • The toolbar shows only one "selected" option, which generally is the icon in the sidebar.
  • Generally, the option is stored in a single var. For instance, the Alignments toolbar stores its value in the align attribute, a string that can take a set of possible values: left, middle, and so on. How can we store two alignments combination there?

Help me understand this better. You can choose both full-wide and toggle the full height, with both those options being in the same toolbar-group, no?

Please help me to get rid of the idea of grouping toolbars-group, slightly described in my previous comment. In short, it suggests adding a new organization level, to put their toolbars-group.

I suspect this question has technical underpinnings rather than visual. I will defer to others that I hope will contribute with thoughts here. Thanks for taking a look at this!

@retrofox
Copy link
Contributor

retrofox commented Nov 6, 2020

Bold and italic can be combined, but they remain in the same group. To me, the separation is about hierarchy rather than behavior, and in grouping based on the former, general rules that apply to all blocks can be made.

Oh, right. that's correct. It works because the toolbar is not collapsed.

There is no clear way to show these three selected states (italic, bold, and link) in the same icon when the toolbar is collapsed. That's what I mean with the alignment example. For text alignment, It's possible because no makes sense setting more than one option (left, middle, and right), and the current state is easily shown with the icon in the toolbar.

...the separation is about hierarchy rather than behavior...

Yes.., it's doable putting buttons with different behaviors in the same toolbar, but when it's collapsed, it loses the ability to show the state selected. It happens with the more rich text control toolbar, for instance.

I guess it isn't a big deal since they seem to be secondary options.

Help me understand this better. You can choose both full-wide and toggle the full height, with both those options being in the same toolbar-group, no?

Yes, technically we can but It will require refactoring the block-alignments-toolbar because currently, it supports selecting only one option at the same time. It's stored in the align attribute, which is a string. If we want to pick more than one option, ... maybe we can combine the options in the same string... something like center fullHeight, or replacing the string with an array. Sounds intense :-).

Question about thi approach. Let's suppose we do something like this:

How do we reflect on the toolbar the current option when it's collapsed?
Also, to me, with this disposition, is not clear that we can combine the alignment with the full height option. You suggested some time ago adding a separator between the "regular" alignments and the full height.

Block Layout
- Left
- Center
- Right
- Wide
- Full wide
___________
Additional Layout Effects
- Full-height

Finally, to me, the solution is just was suggested by you:

These could be in a group together:

In other words, Be able grouping toolbar-groups together, in the same section

@jasmussen
Copy link
Contributor Author

Yes.., it's doable putting buttons with different behaviors in the same toolbar, but when it's collapsed, it loses the ability to show the state selected. It happens with the more rich text control toolbar, for instance.

This isn't true in case of alignments, though:

alignment

I think I hear what you're saying — it's not a direct toggle like full-height, because the toggle is inside the dropdown. But I still think the two can coexist.

How do we reflect on the toolbar the current option when it's collapsed?

I don't think we should do that 😅 — exactly because we can't show the state. You shouldn't select multiple alignments that stack, in the same dropdown.

In other words, Be able grouping toolbar-groups together, in the same section

Visually yes that's what I'm after. I believe there may be some trickiness around what gets announced to screenreaders as a single group, that's why a refactor might be necessary 🙈

@retrofox
Copy link
Contributor

retrofox commented Nov 6, 2020

Yes.., it's doable putting buttons with different behaviors in the same toolbar, but when it's collapsed, it loses the ability to show the state selected. It happens with the more rich text control toolbar, for instance.

This isn't true in case of alignments, though:

alignment

I mean "it loses the ability" because it shows only one selected icon. This is tied with:

I don't think we should do that 😅 — exactly because we can't show the state.

So it answers the question if this disposition is correct:

.

So the answer is - no, it isn't correct, because...

You shouldn't select multiple alignments that stack, in the same dropdown.

I need to improve and incorporate the proper vocabulary because I'm making the communication confusing. In the gif above, dropdown and toolbar-group represent almost the same: a Toolbar Group. When it's collapsed, the options are sorted into a dropdown, but technically, dropdown doesn't define a toolbar role but a disposition.

I say this because you asked in the previous comment:

Help me understand this better. You can choose both full-wide and toggle the full height, with both those options being in the same toolbar-group, no?

The answer is yes, but it means that the full height will be sorted together with the other alignments options, in the same dropdown 😅
The following drawing shows what I understand about the pieces of the toolbar:

toolbar-group-02

The dropdown is just a way to show the toolbar items, of a toolbar group.
I think we do need an additional toolbar piece, which I called Toolbar section in the following:

toolbar-group-03

I think I hear what you're saying — it's not a direct toggle like full-height, because the toggle is inside the dropdown. But I still think the two can coexist.

At which level? Not in the same toolbar group but the same toolbar section. In your example:

Screen Shot 2020-11-06 at 10 36 54 AM

Visually yes that's what I'm after. I believe there may be some trickiness around what gets announced to screenreaders as a single group, that's why a refactor might be necessary 🙈

yeah... something to keep exploring and researching.

@jasmussen
Copy link
Contributor Author

I need to work on my verbiage! Thank you for the illustrations, and your patience.

What I'm saying, using these terms, is that block level actions like align, height, link, crop, belong together in a single toolbar section. I hope I got that right 😅

@retrofox
Copy link
Contributor

retrofox commented Nov 6, 2020

I need to work on my verbiage! Thank you for the illustrations, and your patience.

What I'm saying, using these terms, is that block level actions like align, height, link, crop, belong together in a single toolbar section. I hope I got that right 😅

Agree. We are on the same page!!! ✋

The first potential issue that arrives at my mind is when using hooks to implement some toolbars. As you know, we can add the alignments toolbar by setting the align support property for the block. Implementing this toolbar-groups composition via this method doesn't seem to be simple to do. Even more, doable.

However, we can go ahead and try to compose them programmatically. It means to add a <ToolbarSection /> (or whatever) to wrap the toolbar groups, for instance here in the Media&Text block.

@jasmussen
Copy link
Contributor Author

Thank you. Cc @ajlende as I know he's had this challenge, in case he has any ideas.

@ajlende
Copy link
Contributor

ajlende commented Nov 6, 2020

@retrofox did a good explanation of ToolbarGroup, ToolbarItem, and Dropdown—he's done more digging into the problem now than I have.

There's additionally a DropdownMenu component that encapsulates a Button toggle and a NavigatableMenu within a Dropdown component. A collapsed ToolbarGroup is conceptually similar to DropdownMenu, but with the accessibility features for fitting inside of a toolbar.

The problem I see is that the isCollapsed prop in ToolbarGroup is a bad abstraction. It adds functionality that should have come from a new component instead of a new prop.

I imagine ToolbarGroup being deprecated and split into a ToolbarSection and a ToolbarDropdown. ToolbarSection just provides the visual grouping (which I think is the same as what @retrofox is thinking). However, the components nested inside would be ToolbarItem, ToolbarButton, and the new ToolbarDropdown (not ToolbarGroup). ToolbarDropdown would take the place of the collapsed ToolbarGroup.

There's also Reakit to consider for the implementation details. The toolbar in Reakit is split into three components: Toolbar, ToolbarItem, and ToolbarSeparator. We could forego the ToolbarSection and instead just have a separator if we wanted to. I don't know which way would be better, but it's something to consider.

@retrofox
Copy link
Contributor

retrofox commented Nov 6, 2020

I imagine ToolbarGroup being deprecated and split into a ToolbarSection and a ToolbarDropdown. ToolbarSection just provides the visual grouping (which I think is the same as what @retrofox is thinking). However, the components nested inside would be ToolbarItem, ToolbarButton, and the new ToolbarDropdown (not ToolbarGroup). ToolbarDropdown would take the place of the collapsed ToolbarGroup.

Sounds like a nice idea to me.

There's also Reakit to consider for the implementation details. The toolbar in Reakit is split into three components: Toolbar, ToolbarItem, and ToolbarSeparator. We could forego the ToolbarSection and instead just have a separator if we wanted to. I don't know which way would be better, but it's something to consider.

No sure how applicable is this approach with the current API of the block editor, especially talking about the separator.

Even going a little bit forward, wondering how we implement the idea of blocks levels.

If I'm not wrong, we'll have to define new slots, one for each part of the toolbar. So in this way, we could fill the different block-levels with the toolbar that we want.

<BlockLevelControls>
	<BlockAlignmentToolbar />
	<BlockFullHeightAlignmentToolbar />
</BlockLevelControls>

<BlockInlineLevelControls>
	<AlignmentToolbar />
</BlockInlineLevelControls>

@ajlende
Copy link
Contributor

ajlende commented Nov 6, 2020

No sure how applicable is this approach with the current API of the block editor, especially talking about the separator.

Then let's keep with the ToolbarSection instead of ToolbarSeparator. 👍

If I'm not wrong, we'll have to define new slots, one for each part of the toolbar. So in this way, we could fill the different block-levels with the toolbar that we want.

I really like that idea! It would especially help with block filters that add items to the toolbar.

@jasmussen
Copy link
Contributor Author

Happy to report this one has been fixed:

Screenshot 2021-02-26 at 09 40 32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

4 participants