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

Issue/3785 - Add alignment controls to heading block toolbar #5635

Closed
wants to merge 6 commits into from

Conversation

amdrew
Copy link
Contributor

@amdrew amdrew commented Mar 15, 2018

Description

Adds the alignment controls to the toolbar, as discussed in #3785

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise looks good.

It seems we should also remove the alignment controls from the block inspector? If so, I'll update the PR.

I'd like some feedback here. Seems to me at least we'd want to drop the duplicated controls from the sidebar inspector. cc @jasmussen

@@ -117,7 +126,9 @@ export const settings = {
subscript: level,
} ) )
}
/>
>
{ alignmentToolbar }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we tab this once more? (Ideally we should have an ESLint rule for this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@mtias mtias added the Needs Design Feedback Needs general design feedback. label Apr 2, 2018
@jasmussen
Copy link
Contributor

Thanks so much for working on this! Here's a screenshot:

screen shot 2018-04-03 at 08 36 00

This looks good, but it doesn't account for responsive or mobile breakpoints:

screen shot 2018-04-03 at 08 36 06

screen shot 2018-04-03 at 08 37 13

One suggestion detailed in #3785 (comment) is to remove the 3 quick shortcuts to set h2 h3 and h4, and instead move those to the switcher interface. To here:

screen shot 2018-04-03 at 08 37 28

☝️ Imagine headings 1-6 all in that list.

I realize that's a lot more work than just adding the alignments toolbar, and if this proves too much work for you then that's totally fine, we'll see if one of the core devs has time to work on it.

I also agree with Andrew, we should remove both the heading level and alignments from the inspector:

screen shot 2018-04-03 at 08 39 34

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Apr 3, 2018
@amdrew
Copy link
Contributor Author

amdrew commented Apr 4, 2018

@jasmussen I've updated the PR and removed the alignment controls from the block inspector.

Good spotting with the responsiveness, I hadn't considered that. Is placing other inspector controls inside the transform menu a pattern that Gutenberg utilizes with other blocks at mobile device widths?

Though it seems you can scroll horizontally to reach other inspector controls (only tested on iOS simulator for mac), I'm unaware on how other mobile devices treat this - Do they all provide some mechanism to scroll horizontally to reach the other controls? If not, moving the headings into the transform menu does seem like the best compromise.

In any case, I've left the headings alone for now as I'm not sure how to move them inside the transform menu, and only for mobile devices (is there a pre-existing function for this?). If someone can give me some pointers I'll definitely give it a shot.

@jasmussen
Copy link
Contributor

Good spotting with the responsiveness, I hadn't considered that. Is placing other inspector controls inside the transform menu a pattern that Gutenberg utilizes with other blocks at mobile device widths?

In general nothing should be put inside the inspector that is necessary for the basic operation of the block, either on desktop or mobile. In this case, the PR addresses the issue that alignments could be considered necessary for the basic operation of the heading block. I don't disagree with that, and there is consensus that this is the case.

The horizontal scrollbar on mobile is a "last resort" at making sure all controls are available, but it's not a good pattern and it's not scalable. The responsive desktop breakpoint is more of an issue, because you can't scroll there.

There are ideas of putting alignments inside a dropdown, but that's also a bandaid.

The best solution is to be careful what we put in the block toolbar, and make sure it scales well. In this case, I thin the solution is, as previously noted, to remove the headings from the block toolbar and add them as transformations.

Thank you for your work so far, really appreciate it. @gziolo if you have bandwidth, do you think you can look at putting the heading sizes inside the switcher? And @karmatosed do you agree that's the best approach for now?

@amdrew
Copy link
Contributor Author

amdrew commented Apr 4, 2018

Got it, I agree. It's difficult to scroll horizontally on a mobile device unless you know you actually can in the first place.

@jasmussen
Copy link
Contributor

So, here are mockups because they speak louder than words. It's not working quite as well visually, as I'd hoped, but it can serve as a basis for discussion.

heading block toolbar

transformations

I imagine you'd still want to have just a single heading block that you inserted, which would default to H2.

@gziolo
Copy link
Member

gziolo commented Apr 4, 2018

The same applies to other blocks:

screen shot 2018-04-04 at 13 32 57

screen shot 2018-04-04 at 13 33 28

@karmatosed
Copy link
Member

@jasmussen I like these mocks. I do wonder if that's a lot of text in a line and the 'size' is needed, maybe it is. I do like it better than what we have now and think it's worth creating to experience.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented May 10, 2018

The Heading block now has text alignment controls in its block toolbar thanks to #6675. Of course, now the block toolbar is the longest of any block, and likely not very mobile friendly.

I think the heading level/size switcher should be a single button with a dropdown, but should not be merged with the block converter/switcher dropdown.

Here is a (kind of sloppy) mockup I made:
image

(Yeah, that is not an h3 heading in the mockup... I forgot to change the number.)

I think the block converter/switcher should probably use a different icon, both to differentiate it from the heading level/size switcher, and to make its function a bit more clear. Personally, I have always thought it was a little odd that the block converter/switcher used the icon of the current block. I think it should look more like this instead:
image

Maybe you could combine the two icon styles by replacing one of the squares in the previous mockup with the icon of the current block. That could also work.

@gziolo
Copy link
Member

gziolo commented May 16, 2018

This was implemented in #6675, but not fully addressed. There is a follow-up PR where we want to tackle it. Closing this one as a duplicate. @SuperGeniusZeb, thanks for your idea how to approach mobile devices. @amdrew, thanks for your time spent on this PR.

@gziolo gziolo closed this May 16, 2018
@amdrew amdrew deleted the issue/3785 branch May 19, 2018 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants