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

Ability to disable block popover through __experimentalUIParts.hasPopover option. #19922

Merged

Conversation

@yansern
Copy link
Contributor

yansern commented Jan 28, 2020

Description

This PR adds ability to disable block popover through the __experimentalUIParts.hasPopover option.

Why do we need this?

We are using blocks without the need for the block popover, block toolbar and everything around it. The implementation as seen in #18173 only hides them using CSS. While they are not visible visually, the components are still rendered, this causes accessibility issues. When user tabs through the interface, the buttons from the block toolbar is still being focused, even though it is not visible on the screen.

Example Usage:

<BlockList
	__experimentalUIParts={ {
		hasPopover: false,
	} }
/>

Test Instructions

We will use storybook to test if the block popover component isn't rendered when the hasPopover attribute is set to false. Here are the steps:

  1. Inside the file storybook/stories/playground/index.js, modify the element <BlockList /> at line 49 to:
    <BlockList __experimentalUIParts={ { hasPopover: false } } />
  2. Run npm run storybook:dev.
  3. Go to Playground > Block Editor > Default.
  4. Click on the first block (and type something if you like) in the editor.
  5. There should be no block popover displayed.

Screenshot

Without hasPopover: false

image

With hasPopover: false

image

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 28, 2020

What will it be used for?

@yansern

This comment has been minimized.

Copy link
Contributor Author

yansern commented Jan 28, 2020

What will it be used for?

@ellatrix With PR #18173, parts of the block UI is hidden but they are still rendered. This causes accessibility issues, e.g. when a user is tabbing between elements on screen, suddenly a hidden element on the block toolbar gets focused even though it is not visible on the screen. It will be better to be able to prevent the entire toolbar from rendering when needed.

@ellatrix ellatrix requested a review from jorgefilipecosta Jan 28, 2020
@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 28, 2020

This seems fine to me. The only concern I have is that this is not tested. I've run into problems before with these options to disable things and the capturing toolbars not being used by any core blocks and then breaking it.

@yansern

This comment has been minimized.

Copy link
Contributor Author

yansern commented Jan 29, 2020

In this issue Automattic/wp-calypso#38635, we are currently hiding the toolbar using CSS but ideally if the toolbar can programmatically disabled, that will be better.

@yansern yansern changed the title Added ability to disable block toolbar through __experimentalUIParts.hasToolbar option. Ability to disable block popover through __experimentalUIParts.hasPopover option. Feb 4, 2020
@yansern

This comment has been minimized.

Copy link
Contributor Author

yansern commented Feb 4, 2020

This seems fine to me. The only concern I have is that this is not tested.

@ellatrix I have added test instructions in the updated PR.

@jorgefilipecosta I have updated this PR from disabling the block toolbar hasToolbar to disabling the block popover hasPopover.

Let us know if there are any concerns to address, hopefully we can get this approved!

Copy link
Contributor

ockham left a comment

This is working nicely in my testing! 🎉

I left a few small suggestions that I think would make these changes more aligned with prior art 🙂

Co-Authored-By: Bernie Reiter <ockham@raz.or.at>
@ockham
ockham approved these changes Feb 5, 2020
Copy link
Contributor

ockham left a comment

Looking good, and still working well.

Since this only modifies something __experimental, I think we can go ahead and merge, unless someone is strongly opposed (cc @ellatrix and @jorgefilipecosta).

@ockham ockham merged commit fd1da51 into WordPress:master Feb 6, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Feb 6, 2020

Not sure what changed since I last saw this PR. What happens if the "top toolbar" option is enabled? I think the toolbar will still show in the main top toolbar no?

@yansern

This comment has been minimized.

Copy link
Contributor Author

yansern commented Feb 6, 2020

Just some context here for clarity, the current component tree looks like this:

BlockPopover
|_ BlockToolbar
|_ BlockBreadcrumb
|_ Inserter

Not sure what changed since I last saw this PR.

We realized that disabling the BlockToolbar isn't enough, so the scope of this PR has changed from disabling just the BlockToolbar to disabling the BlockPopover. And the option name has changed from from hasToolbar to hasPopover.

What happens if the "top toolbar" option is enabled? I think the toolbar will still show in the main top toolbar no?

Does your "top toolbar" refer to the BlockToolbar/BlockPopover? If so, the BlockToolbar/BlockPopover is enabled by default (as it is), it only doesn't show when hasPopover is set to false.

chipsnyder added a commit that referenced this pull request Feb 7, 2020
To allow disabling the block popover (which includes the block toolbar and inserter).
@ellatrix ellatrix added this to the Gutenberg 7.5 milestone Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.