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

[WIP] Core blocks: Refactor heading styles to use dropdown #6781

Closed
wants to merge 2 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 16, 2018

Description

Related issue: #783.

This PR tries to fix a regression introduced in #6675. On mobile devices, some controls are hard to access because there are too many of the rendered in the toolbar.

The proposed solution was described by @SuperGeniusZeb in #5635 (comment)

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.

How has this been tested?

Manually.

Screenshots

It still needs more ❤️ .

screen shot 2018-05-16 at 16 17 38

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress Needs Design Feedback Needs general design feedback. labels May 16, 2018
@gziolo gziolo added this to the 2.9 milestone May 16, 2018
@gziolo gziolo self-assigned this May 16, 2018
@gziolo
Copy link
Member Author

gziolo commented May 16, 2018

So far, I mirrored what we have for BlockSwitcher component and applied subscript for headings. We might want it to align closer to Toolbar component if we plan to use this in other places. Most of the styles are missing, this is something that is planned next.

@jasmussen
Copy link
Contributor

I pushed a few changes so it now fits on an iPhone. Before:

before

After:

screen shot 2018-05-17 at 09 37 25

I also polished the subscript a bit.

There's still a bit of messiness with the dropdown:

screen shot 2018-05-17 at 09 37 51

But perhaps this is faster to fix than reverting? We can still look at improving the "style selector" at a later time, but this at least fixes the regression.

@gziolo gziolo modified the milestones: 2.9, 3.0 May 17, 2018
@gziolo
Copy link
Member Author

gziolo commented May 17, 2018

Yes, let's move to 3.0. Reverting the original issue with #6794.

@gziolo gziolo force-pushed the update/heading-styles-fix branch from 1c75812 to 37e7434 Compare May 17, 2018 08:39
@gziolo
Copy link
Member Author

gziolo commented May 17, 2018

I rebased this branch with the latest changes in master (#6974 revert mostly).

@karmatosed and @mtias, this one needs your feedback before we can continue future iterations. As @jasmussen mentioned:

There's still a bit of messiness with the dropdown

We could copy styles from the block switcher to make it work, but we want to provide a long-term solution which doesn't introduce too much code duplication. So let's agree on the final approach first before we take a deep dive into refactorings. The main question is if we want to have this drop-down as proposed, how much it should share logic with the block switcher? Or should we rather have an independent style switcher which could be used for other blocks (Quote?)?

@jasmussen
Copy link
Contributor

Matías, I recall from conversations I've had with you that you've never been happy with the block variation UI (see also #5947). I share that feeling. Do you think a variation picker drop-down as proposed here could be a solution?

@karmatosed
Copy link
Member

karmatosed commented May 17, 2018

I have to admit I'm not wild about seeing a double 'H', I understand what this solves, but I'm not super sure a double drop down is the way forward. I am happy to be wrong there though. Maybe one iteration could be to not have double H somehow?

2018-05-17 at 10 44

If you look above it's a little less confusing than seeing the double H dropdown's beside each other.

@youknowriad
Copy link
Contributor

Just a crazy idea, what if we leverage the transform API to transform the block to it-self using modified attributes. This API doesn't exist at the moment, but this would allow us to show "Heading 1" ... "Heading N" in the block switcher dropdown.

@ZebulanStanphill
Copy link
Member

@karmatosed I suggested changing the block switcher icon to something different. (See initial post.) You could also replace one of the squares in that design with the icon of the current block so the block switcher could still double as a hint of what the current block is.

block-switcher-mockup-thing

@youknowriad There was a mockup of that sort of thing here:
#5635 (comment)

@jasmussen
Copy link
Contributor

#5635 (comment)

We could even go this route without the actual heading size changes being shown, and simply show H1-H6 in the block switcher. This would be simpler, and I think the most obvious way to go. But at the same time, I like @mtias thinking that we need a good UI for showing variations of a block, a pattern we can reuse for quotes in #5947. But I'm coming up short for what it should be, because you definitely need this variation chooser to be part of the block toolbar — we can't hide that in the sidebar.

@gziolo
Copy link
Member Author

gziolo commented May 18, 2018

Just a crazy idea, what if we leverage the transform API to transform the block to it-self using modified attributes. This API doesn't exist at the moment, but this would allow us to show "Heading 1" ... "Heading N" in the block switcher dropdown.

Yes, this might be even simpler to implement if we add another API in addition to transforms -> styles, which would have their own group in the block switcher as proposed by @jasmussen.

@ZebulanStanphill
Copy link
Member

I was initially opposed to having the heading type and transform options in the same dropdown, since it felt like an odd special-case solution. However, considering that the Quote block has similar block variations and other blocks may also implement this sort of thing in the future, and since the different block types are still separated from variations by the “Transform into:” text, I have changed my mind.

I now think the single dropdown is the best solution. Switching heading size (or quote style) and transforming the block are pretty similar, and having them in a single dropdown both eliminates any confusion of 2 similar-looking dropdowns and also makes the block toolbar smaller... the same size as the Paragraph toolbar.

@karmatosed
Copy link
Member

I have to say I really like leveraging the transform API in the way @gziolo you show in the mock, great idea @youknowriad.

@chrisvanpatten
Copy link
Contributor

Oh wow, that's great! Doing the same in the quote block (allowing multiple "styles" under the block transform menu) would be a great other use-case for this!

@ZebulanStanphill
Copy link
Member

Relevant: #7227. See also the recent discussion in #783.

@gziolo
Copy link
Member Author

gziolo commented Jun 8, 2018

@karmatosed, I’m closing this one given that we came out with a better idea how to tackle this issue 🎉

@gziolo gziolo closed this Jun 8, 2018
@gziolo gziolo deleted the update/heading-styles-fix branch August 30, 2018 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants