-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Try/2031 manual focus #2392
Try/2031 manual focus #2392
Changes from all commits
aa66504
14c9446
8bd904a
fdc699b
7be9165
2f86df0
13d9cc0
7664de5
763e186
a99096e
8cc32a2
c05dc9c
394371c
829af3f
c210a69
2770127
fdc1120
5ae9a14
6574e4b
9dd13cf
c70d4f8
20218b0
36bb0d1
7924dd7
3432da5
b1f5896
b31ffee
16021fa
aa77c25
033d0f3
69f5011
84b183c
1facd3c
c8f7363
f3af11c
0e533c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,13 @@ | |
*/ | ||
import { connect } from 'react-redux'; | ||
import { uniq, get, reduce, find } from 'lodash'; | ||
import clickOutside from 'react-click-outside'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
import { Component } from '@wordpress/element'; | ||
import { Dashicon, IconButton } from '@wordpress/components'; | ||
import { Toolbar, DropdownMenu } from '@wordpress/components'; | ||
import { getBlockType, getBlockTypes, switchToBlockType } from '@wordpress/blocks'; | ||
|
||
/** | ||
|
@@ -21,33 +20,9 @@ import { replaceBlocks } from '../actions'; | |
import { getBlock } from '../selectors'; | ||
|
||
class BlockSwitcher extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
this.toggleMenu = this.toggleMenu.bind( this ); | ||
this.state = { | ||
open: false, | ||
}; | ||
} | ||
|
||
handleClickOutside() { | ||
if ( ! this.state.open ) { | ||
return; | ||
} | ||
|
||
this.toggleMenu(); | ||
} | ||
|
||
toggleMenu() { | ||
this.setState( ( state ) => ( { | ||
open: ! state.open, | ||
} ) ); | ||
} | ||
|
||
switchBlockType( name ) { | ||
return () => { | ||
this.setState( { | ||
open: false, | ||
} ); | ||
this.props.onTransform( this.props.block, name ); | ||
}; | ||
} | ||
|
@@ -72,38 +47,21 @@ class BlockSwitcher extends Component { | |
} | ||
|
||
return ( | ||
<div className="editor-block-switcher"> | ||
<IconButton | ||
className="editor-block-switcher__toggle" | ||
icon={ blockType.icon } | ||
onClick={ this.toggleMenu } | ||
aria-haspopup="true" | ||
aria-expanded={ this.state.open } | ||
label={ __( 'Change block type' ) } | ||
> | ||
<Dashicon icon="arrow-down" /> | ||
</IconButton> | ||
{ this.state.open && | ||
<div | ||
className="editor-block-switcher__menu" | ||
role="menu" | ||
tabIndex="0" | ||
aria-label={ __( 'Block types' ) } | ||
> | ||
{ allowedBlocks.map( ( { name, title, icon } ) => ( | ||
<IconButton | ||
key={ name } | ||
onClick={ this.switchBlockType( name ) } | ||
className="editor-block-switcher__menu-item" | ||
icon={ icon } | ||
role="menuitem" | ||
> | ||
{ title } | ||
</IconButton> | ||
) ) } | ||
</div> | ||
} | ||
</div> | ||
<Toolbar> | ||
<li> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aside: Ideally a consumer of the <ul>
{ React.Children.map( ( child, i ) => (
<li key={ i }>{ child }</li>
) ) }
</ul> Mentioning as aside since it's not really relevant to the scope of your pull request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would make using the Toolbar with children slightly easier (and I have considered it), however it would also remove some control from the caller (ie to add classes to some li elements). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To me, this is not control we want to allow, or at least not through exposing Ultimately, I don't think we know enough about the actual use cases to need to concern ourselves with this control just yet though. |
||
<DropdownMenu | ||
icon={ blockType.icon } | ||
label={ __( 'Change block type' ) } | ||
controls={ | ||
allowedBlocks.map( ( { name, title, icon } ) => ( { | ||
icon, | ||
title, | ||
onClick: this.switchBlockType( name ), | ||
} ) ) } | ||
tabIndex="-1" | ||
/> | ||
</li> | ||
</Toolbar> | ||
); | ||
} | ||
} | ||
|
@@ -120,4 +78,4 @@ export default connect( | |
) ); | ||
}, | ||
} ) | ||
)( clickOutside( BlockSwitcher ) ); | ||
)( BlockSwitcher ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these changes, what are the implications on block authors for managing tab index? How do we document this? Can we avoid it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to hijack the tab keypress to avoid the default tabIndex behavior when in the tabbable cycle, so that we don't need buttons to be assigned with
tabIndex="-1"
?