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

Heading block: move alignment controls to toolbar #16682

Closed
wants to merge 7 commits into from

Conversation

@draganescu
Copy link
Contributor

commented Jul 19, 2019

Description

Closes #16648

How has this been tested?

Tested locally.

Screenshots

Screenshot 2019-07-19 at 17 56 01

Types of changes

Added an alignment toolbar component to the heading block's toolbar.
I think we should also remove them from the inspector, right @mapk ?

@paaljoachim

This comment has been minimized.

Copy link

commented Jul 24, 2019

This issue would then also be associated with moving alignment options of the heading to the toolbar.

Try: Always collapse block alignments.
#16557

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

I think we should also remove them from the inspector, right @mapk ?

Yep, let's do that. Right now I'm seeing them doubled up. Let's just remove them all now that they are included in the toolbar.

Screen Shot 2019-07-24 at 4 09 52 PM

@gziolo

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Yep, let's do that. Right now I'm seeing them doubled up. Let's just remove them all now that they are included in the toolbar.

Yes, they should be removed from the sidebar with the proposed change.

We should also make sure that this PR takes into account changes proposed in #16557. I guess it would make sense to land the latter first and then apply it here.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

This is looking good overall. A few things about consistency and collapsing.

  • The text alignment toolbar is not collapsed in both the paragraph and heading blocks at the root level
  • The text alignment toolbar is collapsed in both the paragraph and heading blocks inside the columns block.
  • The heading level toolbar is not collapsed no matter where we use the blocks.

Questioins

  • Should we just collapse all these toolbars in all situations especially since we're adding a lot of tools to a lot of blocks?
  • If not, should we align the "heading level" toolbar behavior on the text alignment toolbar as well: collapsed inside columns and keep uncollapsed at the root level?

Thoughts @mapk @mtias @jasmussen @kjellr

@gziolo

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@youknowriad, see this comment under PR I lined earlier today: #16557 (comment)

I think this work started by @jasmussen together with my updates addressed most of the issues you listed.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Should we just collapse all these toolbars in all situations especially since we're adding a lot of tools to a lot of blocks?

Yes. I tested @gziolo's additional commits to that PR and the collapsed toolbar elements didn't bother me. It felt natural to open them up even when there was more than one to interact with.

@paaljoachim

This comment has been minimized.

Copy link

commented Jul 26, 2019

When there are alternatives to a specific tool (in the toolbar) associated with doing a similar thing it becomes natural to have them in a drop down. The drop down shows variation of one specific feature.

The additional More drop down signals an area that has a mix of tools not so often used, but good to have available.

@paaljoachim

This comment has been minimized.

Copy link

commented Jul 26, 2019

Btw @draganescu we should also have the h tags in a drop down in the toolbar.

#15096 (comment)

@mcsf mcsf changed the title added alignment to the toolbar for consistency Heading block: move alignment controls to toolbar Jul 26, 2019

@gziolo gziolo referenced this pull request Jul 29, 2019
0 of 5 tasks complete

@draganescu draganescu requested a review from ellatrix as a code owner Aug 2, 2019

@draganescu draganescu force-pushed the try/unify-formatting-tools branch from 1ab31f2 to 29c994d Aug 2, 2019

@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

I have removed the align tools from the inspector, added a collapsing item in the toolbar for the heading levels and kept the heading levels always expanded in the inspector.

The problem is that the heading collapsed icon is very confusing:

Screenshot 2019-08-02 at 21 07 53

I am unsure what to use instead as the "H" icon is already there in the block switcher, @mapk any suggestions :D

@paaljoachim

This comment has been minimized.

Copy link

commented Aug 2, 2019

I am looking at what it might look like. I am sure not sure if this is a good solution though.

h-tag-drop-down

Having two H drop downs next to each other can be somewhat confusing.

@drw158

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@paaljoachim I like your solution because it shows which level heading is selected. An additional enhancement would be to tweak the H icon on the left to something like this maybe (imagine an H instead the letters shown):
1
Screen Shot 2019-08-02 at 3 52 13 PM
2
Screen Shot 2019-08-02 at 3 51 24 PM
3
Screen Shot 2019-08-02 at 3 49 50 PM
4
Screen Shot 2019-08-02 at 3 49 11 PM
5
Screen Shot 2019-08-02 at 3 48 44 PM
6
Screen Shot 2019-08-02 at 3 48 18 PM

My favorite is 2 or a variation of that idea.

@paaljoachim

This comment has been minimized.

Copy link

commented Aug 2, 2019

Here is the earlier T that is still in use in the Core version of Gutenberg. With a Heading drop down next to it.

Earlier-version-h-tag-drop-down

Screen Shot 2019-08-03 at 01 39 12

Hey Dave
@drw158
Here are the icons you shared added into the wireframe.

ex1-h-tag-drop-down

ex2-h-tag-drop-down

ex3-h-tag-drop-down

ex4-h-tag-drop-down

ex5-h-tag-drop-down

ex6-h-tag-drop-down

@drw158

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Thanks for the mockups, that helps.I think the decision to use an H instead of T is still sound, so we should keep the H. We should be able to create a new icon incorporating some of those ideas to make the icon look more like a "group of heading options".

Screen Shot 2019-08-06 at 2 22 52 PM

@paaljoachim

This comment has been minimized.

Copy link

commented Aug 6, 2019

Here are additional mockups.

Gutenberg-Heading-dropdown-h-lines-black-border

Gutenberg-Heading-dropdown-2-boxes

Gutenberg-Heading-dropdown-3-stripes

Gutenberg-Heading-dropdown-3-stripes-box

Gutenberg-Heading-dropdown-h-lines

Gutenberg-Heading-dropdown-many-lines

Gutenberg-Heading-dropdown-many-lines-black-box

@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

I think 5 works best as usually headings clear floats and content is below. Also the other resemble drop cap too much.

@richtabor

This comment has been minimized.

Copy link

commented Aug 7, 2019

I am looking at what it might look like. I am sure not sure if this is a good solution though.

h-tag-drop-down

Having two H drop downs next to each other can be somewhat confusing.

As long as the H2 icon adapts to switchever is active I’d say this is ok. The hierarchy between the H and following Hx icons works fine here. I vote for keeping the H heading icon as is.

@draganescu draganescu reopened this Aug 11, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Have you tried to make SVG icon more dynamic using text or something similar for numbers?

@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

there is no bg of the heading dropdown toggle, although no toolbar button behaves like that

Good catch, @draganescu! I'd say don't show the background color in the top toolbar level. This way it's consistent with all other toolbar dropdowns. Basically, keep it as you have it.

@draganescu draganescu removed the Needs Dev label Aug 15, 2019

@draganescu draganescu self-assigned this Aug 15, 2019

@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Have you tried to make SVG icon more dynamic using text or something similar for numbers?

No, but that should be something to check out too. Not sure if in this PR. The H icons are half SVG half subscript :)

@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

This looks ready to go. @gziolo @mapk what do you think?

@mtias

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Thanks for the mockups, that helps.I think the decision to use an H instead of T is still sound, so we should keep the H.

I'm still against using "H" here because, while standard in terms of HTML tags, it perpetuates using English as the canonical source of meaning — the H on its own can be really confusing in other languages for people not familiar with HTML. (I'm totally fine with H being used for the hierarchy level as it's more technical.)

I like some of the ideas proposed by @drw158 for the icon as it gives a bit more context.

Also, the dropdown here seems like a perfect case for showing the actual size rendering of the heading like we used to do on the font-size dropdown. (cc @enriquesanchez)

It would also be a great place to pre-warn if you are choosing an incorrect heading level for the current context.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

Also, the dropdown here seems like a perfect case for showing the actual size rendering of the heading like we used to do on the font-size dropdown.

This is a great point! All the more reason to get this selectbox right in #16473 (comment)

@richtabor

This comment has been minimized.

Copy link

commented Aug 27, 2019

Also, the dropdown here seems like a perfect case for showing the actual size rendering of the heading like we used to do on the font-size dropdown. (cc @enriquesanchez)

I lean towards not having actual size rendering of headings here. UI wise, that implementation could lead to display issues for headings that may not follow "standard" sizes. For example, how would we account for actual heading sizes if a theme opts for rather large headings.

I see value in having the sizes there, but the cognitive load may not be worth it - likely over complicating the component.

I could be convinced otherwise though - I'd appreciate any further insight. :)

@paaljoachim

This comment has been minimized.

Copy link

commented Aug 27, 2019

It seems this PR which seemed straight forward now has added elements.

  1. A suggestion on not using the H but another icon.
  2. Actual size rendering.

These are additional elements that we can get back to later on. Lets get this merged as it is right now as a first iteration.

Uses the H in the Heading drop down. (H2, H3, H4 etc)
So that we go from this:

Screen Shot 2019-08-27 at 23 41 24

To something like this:

Gutenberg-Heading-dropdown-H-Alignment

Thanks.
Then we can after this PR is merged create a new issue for switching the icon and another issue for having actual size rendering of the Heading drop down options.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

I can't get this PR running locally, and so am leaving a proper review to someone who can.

In light of @paaljoachim's comment above, I'd agree that we should keep this PR focused for now. Moving the heading levels into the dropdown while showing the active heading level in the toolbar's icon looks like a reasonable solution since most everything else in the toolbar is also in a dropdown nowadays.

I'd rather not change the Heading icon back to something else. There was good discussion about this here: #15340 and seemed to fulfill a11y needs better. I don't believe the "T" was any more cross-language compatible than an "H". And the "H" at least jives with the Heading levels. So let's keep it as is. That being said, I'm totally okay with expanding on the "H" and iconizing it more. But that's another PR.

@paaljoachim

This comment has been minimized.

Copy link

commented Aug 30, 2019

I believe this PR is ready for a Code Review.

packages/block-library/src/heading/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/heading/heading-toolbar.js Outdated Show resolved Hide resolved
@@ -43,6 +43,13 @@ function DropdownMenu( {
}
}

let activeControl = false;
flatMap( controlSets, ( controlSet ) => {

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 30, 2019

Member

It seems like a very specific implementation of the heading dropdown. Can you think of an alternative approach where DropdownMenu stays unmodified so we could leave this component as is? Ideally, all those CSS styles are applied directly to HeadingToolbar components or alternatively, we modify SVG icons to take the number as param.

This comment has been minimized.

Copy link
@draganescu

draganescu Sep 4, 2019

Author Contributor

Maybe, but that would complicate this PR. I tried to follow the same pattern in the interest of pushing the feature forward. We do already use isActive in the DropdownMenu so I don't feel I've introduced specific Heading related behavior, just some logic to determine the active DropdownMenu element.

I will look into implementing the heading icons differently and get rid of the subscript in the heading component but we'll see. I wonder if there is any benefit of still using subscript for these kinds of icons?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 12, 2019

Contributor

Can you think of other use cases for subscripts like these?

@paaljoachim

This comment has been minimized.

Copy link

commented Sep 6, 2019

Hopefully we can get this merged into the next version of Gutenberg. It will have an important effect on the heading UI.

@gziolo @draganescu @mtias

@@ -43,6 +43,13 @@ function DropdownMenu( {
}
}

let activeControl = false;
flatMap( controlSets, ( controlSet ) => {

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 12, 2019

Contributor

We shouldn't be using flatMap if we don't need the return value.

@@ -43,6 +43,13 @@ function DropdownMenu( {
}
}

let activeControl = false;
flatMap( controlSets, ( controlSet ) => {
activeControl = find( controlSet, ( control ) => {

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 12, 2019

Contributor

Can we break after finding the active control?

@@ -43,6 +43,13 @@ function DropdownMenu( {
}
}

let activeControl = false;
flatMap( controlSets, ( controlSet ) => {

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 12, 2019

Contributor

Can you think of other use cases for subscripts like these?

}

&:not(:disabled):not([aria-disabled="true"]):not(.is-default).is-active > svg {
@include formatting-button-style__active();
}

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 12, 2019

Contributor

Looks like this new line snuck in.

@gziolo gziolo referenced this pull request Sep 12, 2019
0 of 1 task complete
@draganescu

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

@epiqueras thanks for the review. I will close this in favor of #17419 which rightfully removes the whole subscript implementation.

@draganescu draganescu closed this Sep 13, 2019

@talldan talldan deleted the try/unify-formatting-tools branch Sep 13, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

@draganescu, many thanks for all work on this PR 🙇

#17419 is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.