Show transforms as a vertical list #23028

ntsekouras · 2020-06-09T12:58:02Z

Description

Resolves #22073

In this PR:

Transforms are shown as a vertical list, so as to help with familiarity with other editors.
Transforms and Block Styles are combined in a drop down menu and arrow navigation is supported.
Styles have been moved below transforms.
Previews have been implemented with a Popover and changed style.
Added support for role attribute in BlockStyles component.
Enhanced Styles Preview at smaller viewport ( 782px ), when admin bar goes big

How has this been tested?

Screenshots

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.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

youknowriad · 2020-06-09T14:54:43Z

Cool stuff here Nick.

I'm testing on safari and I noticed two small things:

There's some kind of border under the first item.

Feels like the padding around the whole dropdown is bigger than the other popovers?

ntsekouras · 2020-06-09T15:22:37Z

There's some kind of border under the first item.

I will fix that. It's the focused style I see.

Feels like the padding around the whole dropdown is bigger than the other popovers?

Yes, That was the padding from before (grid). I will change it to match the other vertical lists padding.

Thanks @youknowriad!

youknowriad

This is looking good to me overall but there's something I was wondering.

We're not using a DropdownMenu here and instead we use just Dropdown, that's fine cause sometimes you want extra things in the content (like the previews), but we lose some DropdownMenu features like arrow navigation. it makes me wonder if we should wrap the MenuGroups inside NavigableToolbar to enable the arrow navigation.

jasmussen · 2020-06-10T08:20:12Z

This is nice! Thanks for working on this. Here's what I see:

At the end, instead of using tab to navigate the menu items, I use arrowkeys, which breaks. That feels a bit frustrating, because arrowkeys work, for example, in the ellipsis menu. This would be nice to address.

Designwise, I think we should try to move closer to the mockup shown in #18667 (comment):

It doesn't have to be part of this PR — but it's worth sharing the source reason for making into a list. One small step towards that could be to change this:

	.block-editor-block-switcher__label {
		margin-bottom: $grid-unit-10;
		color: $medium-gray-text;
	}

to this

	.block-editor-block-switcher__label {
		margin-bottom: $grid-unit-15;
	}

That would get us very close even just with the list.

ntsekouras · 2020-06-10T08:38:44Z

We're not using a DropdownMenu here and instead we use just Dropdown, that's fine cause sometimes you want extra things in the content (like the previews), but we lose some DropdownMenu features like arrow navigation. it makes me wonder if we should wrap the MenuGroups inside NavigableToolbar to enable the arrow navigation.

Hey @youknowriad! I thought about arrow navigation and although I didn't implement it in this PR, I had seen the Autocomplete component as some reference that implements arrow navigation itself .

Could the arrow navigation be implemented with the way you suggest also here?

Maybe others share their thoughts too?

ntsekouras · 2020-06-10T08:55:04Z

Designwise, I think we should try to move closer to the mockup shown in #18667 (comment):

I thought that was preferred to move Styles below, as mentioned in the issue. Should we revert them as they were before on top?

At first I implemented the lists closer to the first mockup ( more paddings etc ). But as mentioned from @youknowriad and I believe he is right, this wasn't so consistent with the rest vertical lists. After that I made it more compact and to match the hover styles etc.

Also I am wondering if it would make sense to add another level for the user to find out the available Style options, since there is a small number of styles?

Thanks for reviewing @jasmussen! I have also made the css fix you suggested.

jasmussen · 2020-06-10T09:11:25Z

I thought that was preferred to move Styles below, as mentioned in the issue. Should we revert them as they were before on top?

Ah, thanks for surfacing that:

I think it's fine to try with the styles below for starters. I do think we lost a little bit from this mockup:

A few things can probably get us closer:

Increase the padding inside both menus to be larger. Let's try 18px for now — ideally we should use variables that adhere to the 24px grid, but there's some overlap with the inner padding of menu items, and 18px seems like a good start.
Separator should be $dark-gray-primary
Preview should be spaced better — top: -$border-width;, and left: calc(100% + $grid-unit-15); seems to work well
.block-editor-block-switcher__preview-title should be color: $dark-gray-primary
.block-editor-block-switcher__popover .components-popover__content .block-editor-block-switcher__preview should have 18px padding instead of 10 to match what we're trying above
.block-editor-block-styles__item-label should also have zero left padding.

That gets us this, which I think is a good start:

Let me know if you'd like me to push some changes directly to your branch.

ntsekouras · 2020-06-10T09:37:18Z

I think it's fine to try with the styles below for starters. I do think we lost a little bit from this mockup:

I had it like this in my first iterations but as pointed out it was inconsistent with other vertical "menus": Do you think that as well designwise?

Also @jasmussen please take a look to the changes when smaller viewport. Previously they were just not working well with the Styles Preview and I have placed Preview at the bottom.

Let me know if you'd like me to push some changes directly to your branch.

Feel free to push them if you have already made them to test the UI. Just let me know. Thank you!

I am now implementing the arrow navigation but since we have two separate MenuGroups and the focus goes to Blocks, arrow navigation will cycle through Blocks only and not the Styles. Any thoughts/workaround on this @youknowriad ?

jasmussen · 2020-06-10T10:32:35Z

Pushed polish that addresses the feedback I left:

There needs to be contrast between menu item section labels (Styles) and the menu items (Default, Large), so I moved from the styling that's present in the global more menu, to the styling used for categories in the inserter. Would be good to make this consistent across all three, with the same gray color.

jasmussen · 2020-06-10T10:36:57Z

I noticed an error in the console:

index.js:138 Uncaught TypeError: Cannot read property 'style' of undefined
    at Object.onFrame (index.js:138)
    at update (web.cjs.js:516)

That was testing the quote block.

ellatrix · 2020-06-10T10:41:05Z

@jasmussen I think that's caused by another PR recently merged with master. I'll have a look.

ellatrix · 2020-06-10T10:56:32Z

When I arrow navigate, the main scroll container scroll up/down.

ntsekouras · 2020-06-10T10:56:41Z

Would be good to make this consistent across all three, with the same gray color.

@jasmussen do you mean changing the borders here to the same color as Block Switcher?

ntsekouras · 2020-06-10T11:59:56Z

When I arrow navigate, the main scroll container scroll up/down.

It doesn't happen to me. Can you please share your browser info @ellatrix ?

ellatrix · 2020-06-10T13:19:27Z

@ntsekouras It's in chrome. The admin menu on the left side goes up and down as you navigate.

jasmussen · 2020-06-11T08:24:17Z

do you mean changing the borders here to the same color as Block Switcher?

I did not, but we should. Pushed a fix:

Important to only do it for the dark popovers, so we still have the light ones here:

ntsekouras · 2020-06-11T08:50:16Z

Thank you @jasmussen! It's great that you share some details about the design decisions. It's really valuable!! :)

jasmussen · 2020-06-11T08:51:46Z

Always happy to help, ping me anytime. I worked on some of the design implementation of #18667, which isn't actually complete yet, so I'm happy to see every little bit getting us closer to completing it! Thanks again for the PR.

Create seperate BlockTransformationsMenu component

Removed some unnecessary styles.

A node with skipchildrenfocus can skip its children to be able to get focus, fixing some arrow navigation issues. This is a temp commit to get feedback.

tabIndex must be properly discussed to decide how to handle elements not navigable but focusable

ellatrix · 2020-06-22T12:38:19Z

Great work! Let's merge and iterate if there's some remaining issues. It has some time in master for testing.

gziolo · 2020-06-22T13:26:51Z

packages/block-editor/src/components/block-switcher/block-transformations-menu.js

+				return (
+					<MenuItem
+						key={ name }
+						className={ getBlockMenuDefaultClassName( name ) }


Is this class necessary here?

This class is used in BlockTypesList ( it was used before ). I came across some failing e2e tests that were using the transform functionality and this class seemed a good idea to preserve, as it might be used by other e2e tests in the future.

I don’t think that e2e test should shape production code, and I’m sure you can use accessible labels to achieve the same result as block names are unique. It isn’t a big issue but if we can remove unused code with little effort we should always aim for it.

I don’t think that e2e test should shape production code

I definitely agree with this!

In this specific case though ( MenuItem ) there is not any accessible labels to match, besides the content itself. MenuItem has an info property but this would end up adding stuff again.

So, would it be preferable to search through the content?

Well, the content is the accessible label, this is what is going to be announced when users depend on screen readers. By using this approach, we also ensure that UI is accessible. It's harder to target elements by content, but there are many existing tests that do it.

gziolo · 2020-06-22T13:40:09Z

packages/dom/src/focusable.js

@@ -77,7 +92,7 @@ export function find( context ) {
 	const elements = context.querySelectorAll( SELECTOR );

 	return Array.from( elements ).filter( ( element ) => {
-		if ( ! isVisible( element ) ) {
+		if ( ! isVisible( element ) || skipFocus( element ) ) {


Should the description of the function be updated to reflect changes applied? Does it make sense to include an entry in the CHANGELOG for the package? Is it a bug fix?

Is it possible to cover it with a unit test to prevent further regressions?

I missed this comment. Maybe we can create a follow-up PR?

@gziolo It wasn't exactly a bug, more like making DropdownMenu's arrow navigation work properly by adding the BlockStyles, which had nested elements. That was causing problems with the declared Selectors here: https://github.com/ntsekouras/gutenberg/blob/2a2b2c8ce042bc06c220eab6891b0d477b6d9740/packages/dom/src/focusable.js#L20 . Nested elements were also selected as focusables that caused breaking the navigation.

My first thought was introducing a new attribute in this commit ( focusable.js changes ), but suggested to simplify it..

I would really appreciate any feedback for how to handle this better..

focusable is the public API on the @wordpress/dom package: https://www.npmjs.com/package/@wordpress/dom. It has 15k downloads per week. It's used by 3rd party projects:

Using tabindex="-1" – it's a well-established pattern used with keyboard navigation:
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex

A negative value (usually tabindex="-1") means that the element is not reachable via sequential keyboard navigation, but could be focused with Javascript or visually by clicking with the mouse. It's mostly useful to create accessible widgets with JavaScript.

It feels like it was missed in the initial implementation and it would mean a bug fix. @ellatrix, can you confirm?

It was added by Andrew in #2323. Not sure if iframes were meant to be excluded. I don't really understand the change here. Isn't everything that is tab index -1 already excluded? And an iframe's children can't be queried, so they are also excluded?

I think that you need to explicitly mark all children at every nesting level to make it work. It's meant to be addressed with inert attribute but it isn't implemented in all browsers yet.

ntsekouras requested review from ellatrix and youknowriad as code owners June 9, 2020 12:58

youknowriad added General Interface Parts of the UI which don't fall neatly under other labels. Needs Design Feedback Needs general design feedback. labels Jun 9, 2020

ntsekouras changed the title ~~WIP: Show transforms as a vertical list~~ Show transforms as a vertical list Jun 9, 2020

ntsekouras force-pushed the fix/show-transforms-as-vertical-list branch from ff52030 to abb3cc9 Compare June 10, 2020 07:00

youknowriad reviewed Jun 10, 2020

View reviewed changes

youknowriad added the Needs Accessibility Feedback Need input from accessibility label Jun 10, 2020

youknowriad requested review from enriquesanchez and jasmussen June 10, 2020 07:50

ntsekouras requested review from ajitbohra and chrisvanpatten as code owners June 11, 2020 08:23

ntsekouras force-pushed the fix/show-transforms-as-vertical-list branch from 9b52347 to 39391ed Compare June 11, 2020 09:12

ntsekouras added 23 commits June 20, 2020 23:02

Fix aria role related e2e tests

08e3b22

reverse destructuring

a958a9e

Use DropdownMenu instead of Dropdown.

6e092ff

Create seperate BlockTransformationsMenu component

Add class property to support the override styles with DropdownMenu

92fa685

Add role attribute support for BlockStyleItem

37ac84e

Add Preview to a Popover component.

a195fee

Removed some unnecessary styles.

Added container elements to change Preview Popover position.

2b44bf9

Introduce 'skipchildrenfocus' attribute to Focusable.

189b35a

A node with skipchildrenfocus can skip its children to be able to get focus, fixing some arrow navigation issues. This is a temp commit to get feedback.

Add filter for tabindex -1 in focusable

938e1ef

revert BlockTypesList and remove extra role attribute

22f4141

add the Styles Preview popover

3101ccd

Hide popover on mobile + move styles on top

7d7e864

Add labels to MenuGroups

adb4ed2

Wrap icon with BlockIcon

8273999

minor Firefox style fix

83045c3

Rename BlockStyles role to itemRole

c8ac83c

turn PreviewBlockPopover to function component

af166a7

Fix BlockSwitcher tests

b56c05d

Adjust outer block with new Popover functionality

0da2829

make skipFocus function more specific to our use case.

a64b44e

tabIndex must be properly discussed to decide how to handle elements not navigable but focusable

use toLowerCase fn

0cbba47

fix BlockSwitcher e2e tests

50cf323

add block menu default class + fix toolbar roving tabindex e2e tests

2a2b2c8

ntsekouras force-pushed the fix/show-transforms-as-vertical-list branch from 9e434bf to 2a2b2c8 Compare June 20, 2020 20:03

ellatrix merged commit 2ec236b into WordPress:master Jun 22, 2020

github-actions bot added this to the Gutenberg 8.5 milestone Jun 22, 2020

ntsekouras deleted the fix/show-transforms-as-vertical-list branch June 22, 2020 12:41

gziolo reviewed Jun 22, 2020

View reviewed changes

ntsekouras mentioned this pull request Dec 23, 2020

Add block transforms preview #27861

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show transforms as a vertical list #23028

Show transforms as a vertical list #23028

ntsekouras commented Jun 9, 2020 •

edited

Loading

youknowriad commented Jun 9, 2020

ntsekouras commented Jun 9, 2020 •

edited

Loading

youknowriad left a comment

jasmussen commented Jun 10, 2020

ntsekouras commented Jun 10, 2020

ntsekouras commented Jun 10, 2020

jasmussen commented Jun 10, 2020 •

edited

Loading

ntsekouras commented Jun 10, 2020

jasmussen commented Jun 10, 2020

jasmussen commented Jun 10, 2020 •

edited

Loading

ellatrix commented Jun 10, 2020

ellatrix commented Jun 10, 2020

ntsekouras commented Jun 10, 2020

ntsekouras commented Jun 10, 2020 •

edited

Loading

ellatrix commented Jun 10, 2020

jasmussen commented Jun 11, 2020

ntsekouras commented Jun 11, 2020

jasmussen commented Jun 11, 2020

ellatrix commented Jun 22, 2020

gziolo Jun 22, 2020

ntsekouras Jun 22, 2020

gziolo Jun 22, 2020

ntsekouras Jun 23, 2020

gziolo Jun 23, 2020

gziolo Jun 22, 2020

ellatrix Jun 23, 2020

ntsekouras Jun 23, 2020

gziolo Jun 23, 2020

ellatrix Jun 23, 2020

gziolo Jun 23, 2020

Show transforms as a vertical list #23028

Show transforms as a vertical list #23028

Conversation

ntsekouras commented Jun 9, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

youknowriad commented Jun 9, 2020

ntsekouras commented Jun 9, 2020 • edited Loading

youknowriad left a comment

Choose a reason for hiding this comment

jasmussen commented Jun 10, 2020

ntsekouras commented Jun 10, 2020

ntsekouras commented Jun 10, 2020

jasmussen commented Jun 10, 2020 • edited Loading

ntsekouras commented Jun 10, 2020

jasmussen commented Jun 10, 2020

jasmussen commented Jun 10, 2020 • edited Loading

ellatrix commented Jun 10, 2020

ellatrix commented Jun 10, 2020

ntsekouras commented Jun 10, 2020

ntsekouras commented Jun 10, 2020 • edited Loading

ellatrix commented Jun 10, 2020

jasmussen commented Jun 11, 2020

ntsekouras commented Jun 11, 2020

jasmussen commented Jun 11, 2020

ellatrix commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntsekouras commented Jun 9, 2020 •

edited

Loading

ntsekouras commented Jun 9, 2020 •

edited

Loading

jasmussen commented Jun 10, 2020 •

edited

Loading

jasmussen commented Jun 10, 2020 •

edited

Loading

ntsekouras commented Jun 10, 2020 •

edited

Loading