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

Block Switcher: Render disabled button even if multi-selection #13431

Merged
merged 1 commit into from Jan 25, 2019

Conversation

Projects
None yet
4 participants
@aduth
Copy link
Member

aduth commented Jan 22, 2019

Previously: #11600 (comment)

This pull request seeks to remove a condition present in the BlockSwitcher component which causes it to render nothing if there are no possible transformations and there is a multi-selection of blocks.

Without these changes, there is an inconsistency based on an assumption that transformations can only apply to a set of blocks of the same type. While currently true, there is not a guarantee that this will always be the case, and the component itself otherwise makes no expectation that it would be (if transformations exist, a dropdown will be shown with the icon of the first block, regardless of whether all blocks in the selection are of the same type). Additionally, the current behavior would cause nothing to be rendered even if the multi-selection was of blocks of the same type, if there were no multi-transforms for multiple blocks of that type. This is inconsistent with both multi-selections of the same type where a transformation does exist (dropdown shown with icon of that block type), and singular selections of a type where no transformation exists (dropdown not shown, but disabled icon shown of that block type).

Alternatives:

This does present an awkward scenario where multiple blocks of different types shows an icon of the first type:

image

Instead, we may consider:

  • Check earlier in the component whether the multi-selection is of a consistent type, and always return nothing if not. This still makes an assumption that transforms cannot apply to multiple blocks of differing types, but does so consistently. It also allows the disabled icon to be shown for multi-selections of the same type where no transforms exist (e.g. multiple Heading blocks selected).
  • Show a different icon or an aggregate of icons for types in the selection

Testing instructions:

Verify that the BlockSwitcher is shown always, regardless of the blocks composing the selection. The icon should correspond to the first block in the selection, and should be disabled if and only if there are neither valid transforms nor applicable styles to choose from.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 23, 2019

I dig it, thanks for the work. I think it is correct to use a disabled-looking (because it is disabled) icon in the switcher, when no transformation is necessary.

Is it maybe an opportunity to explain what is going on — can a disabled button show a tooltip? Like if you hovered the disabled button, could it say "No transformations available", or something to that effect?

GIF:

gif

It really would be nice if we could swap out that "first of type" icon with something else. Perhaps this?

screenshot 2019-01-23 at 10 31 16

Also, this made me realize (which is completely unrelated to this branch and should not be fixed here) that we are scaling down the block icons! ┻━┻ ︵ヽ(`Д´)ノ︵ ┻━┻

screenshot 2019-01-23 at 10 28 06

The material icons are 24x24, but it's scaled down to 20x20. Which makes them blurry on the majority of screens in the world:

screenshot 2019-01-23 at 10 32 25

When/how did this regress? I feel like we just fixed that recently. I think we might need an e2e test for icons :D

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Jan 23, 2019

@jasmussen those pesky icons! Might be worth looking into screenshot tests for some of the UI stuff that regresses regularly.

I like the idea of a separate icon for multi-block switching that spans multiple block types, but I'm also not too bothered by the first-block-type icon if it's radically easier. I think the benefits here outweigh the costs.

@gziolo gziolo added this to the 5.0 (Gutenberg) milestone Jan 25, 2019

@gziolo

gziolo approved these changes Jan 25, 2019

Copy link
Member

gziolo left a comment

Let's do it 👍

Is it maybe an opportunity to explain what is going on — can a disabled button show a tooltip? Like if you hovered the disabled button, could it say "No transformations available", or something to that effect?

screen shot 2019-01-25 at 14 24 26

We don't do it also for blocks which don't offer transformations. I think it should be tackled separately.

Another thing we could do in its own PR, which I raised in the past, is Edit as HTML mode:

screen shot 2019-01-25 at 14 26 01

It could also render the disabled icon for consistency.

@aduth aduth force-pushed the update/block-switcher-render-multi-block branch from e562007 to 1c52dc1 Jan 25, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jan 25, 2019

I've pushed some updates per feedback, specifically using a different icon when blocks in a selection are not of same type.

Same type, with transforms available (no change from master):

image

Different type, no transforms available (master would have shown nothing):

image

Same type, no transforms available (master would have shown nothing):

image

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 25, 2019

Digging that. I'm sure we might keep at the icon discussions, but for now I think this is lovely. Thanks.

@aduth aduth force-pushed the update/block-switcher-render-multi-block branch from 1c52dc1 to 2aaf4c2 Jan 25, 2019

@aduth aduth merged commit c8cb3c2 into master Jan 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the update/block-switcher-render-multi-block branch Jan 25, 2019

@aduth aduth referenced this pull request Jan 25, 2019

Merged

eslint-plugin: Add rule `no-unused-vars-before-return` #12828

2 of 2 tasks complete

daniloercoli added a commit that referenced this pull request Jan 26, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-enter-key-detection-to-title

* 'master' of https://github.com/WordPress/gutenberg: (29 commits)
  Update for RangeControl documentation (#12564)
  Plugin: Deprecate gutenberg_load_list_reusable_blocks (#13456)
  Update the columns attribute in onSelectImages so that if images are removed via the media modal, the columns can't be higher than the new number of images (#13488)
  Replace the fullscreen "exit" icon with a back arrow (#13403)
  Include :visited links in button color (#12183)
  Amazon Kindle block (#13510)
  Plugin: Deprecate gutenberg_prepare_blocks_for_js (#13457)
  Add watcher on Linux: change fs to node-watch (#13448)
  Plugin: Deprecate `gutenberg` theme support (#13458)
  Datepicker: Add inValidDay support (#12962)
  Block Switcher: Render disabled button even if multi-selection (#13431)
  Plugin: Deprecate gutenberg_register_post_types (#13468)
  Plugin: Deprecate register_tinymce_scripts (#13466)
  Set minimum of words for RSS excerpt (#13502)
  Plugin: Deprecate gutenberg_get_block_categories (#13454)
  Plugin: Deprecate gutenberg_content_block_version (#13469)
  API Fetch: Expose nonce on created middleware function (#13451)
  Plugin: Remove list screens integrations (#13459)
  Plugin: Remove core-defined block detection functions (#13467)
  Spec Parser: Move generated spec parser to package (#13493)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment