-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add block mobile toolbar with move/settings/delete controls. #1872
Conversation
@jasmussen going with this icon for now because we don't have an ellipsis. |
We should have. Try it, |
Btw 😍 |
components/toolbar/index.js
Outdated
@@ -9,7 +9,7 @@ import classnames from 'classnames'; | |||
import './style.scss'; | |||
import IconButton from '../icon-button'; | |||
|
|||
function Toolbar( { controls = [], children } ) { | |||
function Toolbar( { controls = [], children, className = '' } ) { |
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.
classnames
is smart enough to ignore an undefined
value; the default can be removed.
@@ -65,6 +65,34 @@ | |||
transition: 0.2s outline; | |||
} | |||
|
|||
&.is-showing-mobile-controls { | |||
.editor-visual-editor__block-controls > div { |
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.
Relies on slot behavior wrapping with div
. Might be less fragile by targeting anything except mobile tools:
.editor-visual-editor__block-controls > :not( .editor-visual-editor__mobile-tools ) {
} | ||
} | ||
.editor-block-settings-menu { | ||
z-index: 99; |
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.
Should use z-index
function.
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.
It doesn't seem to be needed anymore with the latest approach.
editor/modes/visual-editor/block.js
Outdated
<IconButton | ||
className="editor-visual-editor__mobile-toggle" | ||
onClick={ this.toggleMobileControls } | ||
aria-label={ __( 'Toggle extra block controls' ) } |
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.
Redundant with label
prop below (IconButton
uses label
to apply aria-label
).
editor/modes/visual-editor/block.js
Outdated
className="editor-visual-editor__mobile-toggle" | ||
onClick={ this.toggleMobileControls } | ||
aria-label={ __( 'Toggle extra block controls' ) } | ||
aria-expanded={ true } |
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.
Minor: ={ true }
can be dropped.
editor/modes/visual-editor/block.js
Outdated
aria-expanded={ true } | ||
label={ __( 'Toggle Controls' ) } | ||
> | ||
<Dashicon icon="ellipsis" /> |
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.
IconButton
renders Dashicon
via its icon
prop, i.e. <IconButton icon="ellipsis" />
@@ -364,6 +376,19 @@ class VisualEditorBlock extends Component { | |||
<div className="editor-visual-editor__block-controls"> | |||
<BlockSwitcher uid={ block.uid } /> | |||
<Slot name="Formatting.Toolbar" /> | |||
<Toolbar className="editor-visual-editor__mobile-tools"> | |||
{ ( showUI || isHovered ) && <BlockMover uids={ [ block.uid ] } /> } |
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.
Curious if we need both BlockMover here and above, or if we could apply styles to the root element when 'is-showing-mobile-controls'
is applied to appear as it does with the Toolbar
component.
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.
Tried this but could not get it to work on mobile reliably between proper flex positioning, sticky position, etc.
Going to merge this. |
https://cloudup.com/cBIktw2nLPE
Closes #705.