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

Removed block transforms from the toolbars. #6473

Merged

Conversation

jorgefilipecosta
Copy link
Member

Description

This PR removes the block transforms from the toolbar.
Fixes: #6449

How has this been tested?

Select multiple paragraphs, verify no multi-select transform button appears at the side of Content Structure button (top of the editor).
Select a single paragraph and verify the transforms button does not appear in the toolbar.
Verify both multi-block transforms, and single block ones, work correctly when pressing the "More options" button (three points at the side of the block).

@jorgefilipecosta jorgefilipecosta added the [Type] Enhancement A suggestion for improvement. label Apr 27, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Apr 27, 2018
@karmatosed karmatosed self-requested a review April 27, 2018 18:42
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@jorgefilipecosta jorgefilipecosta force-pushed the update/remove-block-transforms-from-toolbar branch from c59522b to 175d09c Compare April 30, 2018 10:47
@jorgefilipecosta jorgefilipecosta merged commit 731b4fe into master Apr 30, 2018
@jorgefilipecosta jorgefilipecosta deleted the update/remove-block-transforms-from-toolbar branch April 30, 2018 11:00
@jasmussen
Copy link
Contributor

Great to see polish and enhancements, and I do agree with the main purpose of #6449, reducing the number of transform tools.

However in this case I can't help but feel like we should've removed the transformations in the block's More menu, as opposed to the button in the toolbar:

Right now the heading block looks like this (don't mind the bg color):

screen shot 2018-05-01 at 10 45 07

While we can certainly improve that, it does make it harder to unify toolbars across the basic text blocks.

@mtias
Copy link
Member

mtias commented May 1, 2018

I agree, I think we need a better design for the top level transforms before removing them from the toolbar. Too many interactions are going to depend on it.

@karmatosed
Copy link
Member

karmatosed commented May 1, 2018

Whilst I stand by it should be removed from one place and not be in two, I am also in agreement that moving back to just under toolbar area makes sense. That said it should be either not done and some better solution done soon or done as an interim.

@jasmussen
Copy link
Contributor

There are challenges with the switcher as it stands. After discussing with Matías and Tammie in a chat, there are a few challenges with the switcher in the toolbar, one being that it feels like one giant toolbar, when the switcher is perhaps important enough to be grouped separately.

So the toolbar needs a little love, in other words, around these principles:

  • Have only one place for transforms #6449 is still valid — probably best to have a single interface for transformations
  • block transforms is primary action
  • block variation UI needs work (heading sizes, quote types)
  • toolbars should probably only have either block alignments, or text alignments, not both
  • text alignments good to have on h1-6 and p
  • consider better way to group functions in toolbar, so it’s not just one giant toolbar

Those are improvements that are worth looking at as soon as we can, both so we can address #3785, and #5947.


In the mean time, perhaps we should revert or invert this PR. Either simply revert this and bring back the toolbar switcher, or bring back the toolbar switcher and remove the More menu transformations. CC: @jorgefilipecosta

jorgefilipecosta added a commit that referenced this pull request May 3, 2018
We decided that transforms should be a primary action, and we will iterate further to have transforms fit nicely in the toolbars.
@jorgefilipecosta
Copy link
Member Author

Hi @jasmussen thank you for your inputs. A PR that reverts this changes was created #6566.

jorgefilipecosta added a commit that referenced this pull request May 3, 2018
We decided that transforms should be a primary action, and we will iterate further to have transforms fit nicely in the toolbars.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants