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.

Sign up for GitHub

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

Show transforms as a vertical list #23028

Merged
merged 34 commits into from
Jun 22, 2020
Merged

Show transforms as a vertical list #23028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
19374c2
Move Styles below transform options.
ntsekouras Jun 8, 2020
0aab098
revert preview in BlockStyles as they are reused
ntsekouras Jun 8, 2020
d6b93ab
Hide BlockStyles preview only from BlockTypes list.
ntsekouras Jun 8, 2020
577cfd2
Change BlockStyles position next to style options
ntsekouras Jun 9, 2020
8af59ea
Show Preview styles properly and on smaller viewports.
ntsekouras Jun 9, 2020
298275b
Add line break
ntsekouras Jun 9, 2020
8f11e28
Revert changes from other blocks and override in Block Switcher
ntsekouras Jun 9, 2020
baa6541
Add arrow navigation to blocks and BlockStyles
ntsekouras Jun 10, 2020
2a64cfe
Push a little polish.
jasmussen Jun 10, 2020
c9339fe
Fix scrolling window on arrow navigation
ntsekouras Jun 10, 2020
fc42b4b
Destructure items' properties
ntsekouras Jun 10, 2020
08e3b22
Fix aria role related e2e tests
ntsekouras Jun 11, 2020
a958a9e
reverse destructuring
ntsekouras Jun 11, 2020
6e092ff
Use DropdownMenu instead of Dropdown.
ntsekouras Jun 15, 2020
92fa685
Add class property to support the override styles with DropdownMenu
ntsekouras Jun 15, 2020
37ac84e
Add role attribute support for BlockStyleItem
ntsekouras Jun 15, 2020
a195fee
Add Preview to a Popover component.
ntsekouras Jun 15, 2020
2b44bf9
Added container elements to change Preview Popover position.
ntsekouras Jun 16, 2020
189b35a
Introduce 'skipchildrenfocus' attribute to Focusable.
ntsekouras Jun 16, 2020
938e1ef
Add filter for tabindex -1 in focusable
ntsekouras Jun 16, 2020
22f4141
revert BlockTypesList and remove extra role attribute
ntsekouras Jun 16, 2020
3101ccd
add the Styles Preview popover
ntsekouras Jun 16, 2020
7d7e864
Hide popover on mobile + move styles on top
ntsekouras Jun 18, 2020
adb4ed2
Add labels to MenuGroups
ntsekouras Jun 18, 2020
8273999
Wrap icon with BlockIcon
ntsekouras Jun 18, 2020
83045c3
minor Firefox style fix
ntsekouras Jun 18, 2020
c8ac83c
Rename BlockStyles role to itemRole
ntsekouras Jun 18, 2020
af166a7
turn PreviewBlockPopover to function component
ntsekouras Jun 19, 2020
b56c05d
Fix BlockSwitcher tests
ntsekouras Jun 19, 2020
0da2829
Adjust outer block with new Popover functionality
ntsekouras Jun 19, 2020
a64b44e
make skipFocus function more specific to our use case.
ntsekouras Jun 19, 2020
0cbba47
use toLowerCase fn
ntsekouras Jun 20, 2020
50cf323
fix BlockSwitcher e2e tests
ntsekouras Jun 20, 2020
2a2b2c8
add block menu default class + fix toolbar roving tabindex e2e tests
ntsekouras Jun 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/block-editor/src/components/block-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,12 @@ const useGenericPreviewBlock = ( block, type ) =>
[ type.example ? block.name : block, type ]
);

function BlockStyles( { clientId, onSwitch = noop, onHoverClassName = noop } ) {
function BlockStyles( {
clientId,
onSwitch = noop,
onHoverClassName = noop,
itemRole,
} ) {
const selector = ( select ) => {
const { getBlock } = select( 'core/block-editor' );
const { getBlockStyles } = select( 'core/blocks' );
Expand Down Expand Up @@ -141,6 +146,7 @@ function BlockStyles( { clientId, onSwitch = noop, onHoverClassName = noop } ) {
onHover={ () => onHoverClassName( styleClassName ) }
style={ style }
styleClassName={ styleClassName }
itemRole={ itemRole }
/>
);
} ) }
Expand All @@ -156,6 +162,7 @@ function BlockStyleItem( {
onHover,
onSelect,
styleClassName,
itemRole,
} ) {
const previewBlocks = useMemo( () => {
return {
Expand All @@ -182,7 +189,7 @@ function BlockStyleItem( {
} }
onMouseEnter={ onHover }
onMouseLeave={ onBlur }
role="button"
role={ itemRole || 'button' }
tabIndex="0"
aria-label={ style.label || style.name }
>
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-styles/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,5 @@

.block-editor-block-styles__item-label {
text-align: center;
padding: 4px 2px;
padding: 4px 0;
}
40 changes: 40 additions & 0 deletions packages/block-editor/src/components/block-switcher/block-transformations-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { MenuGroup, MenuItem } from '@wordpress/components';
import { getBlockMenuDefaultClassName } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import BlockIcon from '../block-icon';

const BlockTransformationsMenu = ( {
className,
possibleBlockTransformations,
onSelect,
} ) => {
return (
<MenuGroup label={ __( 'Transform to' ) } className={ className }>
{ possibleBlockTransformations.map( ( item ) => {
const { name, icon, title } = item;
return (
<MenuItem
key={ name }
className={ getBlockMenuDefaultClassName( name ) }
Copy link
Member

Choose a reason for hiding this comment

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

Is this class necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

icon={ <BlockIcon icon={ icon } showColors /> }
onClick={ ( event ) => {
event.preventDefault();
onSelect( name );
} }
>
{ title }
</MenuItem>
);
} ) }
</MenuGroup>
);
};

export default BlockTransformationsMenu;
Loading