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

Block Toolbar: Do not hide the block toolbar on mobile #5349

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

youknowriad
Copy link
Contributor

closes #5346

When introducing the viewport module, the block toolbar has been hidden in mobile. I don't know the exact reasons for this but I think we still want it in mobile. So I'm bringing it back here.

Testing instructions

  • select a block in mobile
  • The block toolbar should appear
  • Try in both docker and undocker toolbar.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended Mobile Web Viewport sizes for mobile and tablet devices labels Mar 2, 2018
@youknowriad youknowriad self-assigned this Mar 2, 2018
@youknowriad youknowriad requested review from jasmussen and aduth March 2, 2018 09:52
@jasmussen
Copy link
Contributor

You are THE BEEEST! This fixes it for me. 👍👍👍👍

@aduth
Copy link
Member

aduth commented Mar 2, 2018

Related: #5206

@aduth aduth added this to the 2.3 milestone Mar 2, 2018
@aduth
Copy link
Member

aduth commented Mar 2, 2018

So what was intended by ! isMobile( state ) in this condition (removed as part of #5206)?

/**
* Returns whether the toolbar should be fixed or not.
*
* @param {Object} state Global application state.
*
* @return {boolean} True if toolbar is fixed.
*/
export function hasFixedToolbar( state ) {
return ! isMobile( state ) && isFeatureActive( state, 'fixedToolbar' );
}

@aduth
Copy link
Member

aduth commented Mar 2, 2018

So it appears after these changes, the fixed toolbar appears in mobile. But prior to #5206, even if the "Fixed toolbar" setting was somehow selected (it's hidden on smaller displays), it would still show the contextual toolbar.

@aduth
Copy link
Member

aduth commented Mar 2, 2018

With "Fix Toolbar to Top":

9162e4c (before #5206) fix/mobile-toolbar
before after

@aduth
Copy link
Member

aduth commented Mar 2, 2018

Also, the fixed toolbar section lingers when no block is selected:

image

@@ -29,7 +28,7 @@ function VisualEditor( { hasFixedToolbar, isLargeViewport } ) {
<WritingFlow>
<PostTitle />
<BlockList
showContextualToolbar={ isLargeViewport && ! hasFixedToolbar }
Copy link
Member

@aduth aduth Mar 2, 2018

Choose a reason for hiding this comment

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

I think the issue was a failure to preserve the original condition here.

Previously:

! isMobile( state ) && isFeatureActive( state, 'fixedToolbar' )

This should have translated to:

! isLargeViewport || ! hasFixedToolbar

https://en.wikipedia.org/wiki/De_Morgan%27s_laws

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 tried this condition and it's showing two toolbars on small viewports at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

I tried this condition and it's showing two toolbars on small viewports at the same time.

Only because of the other changes in HeaderToolbar on this branch.

Apply this diff to master, and it behaves as I'd expect:

diff --git a/edit-post/components/visual-editor/index.js b/edit-post/components/visual-editor/index.js
index 1d621863a..a439953e3 100644
--- a/edit-post/components/visual-editor/index.js
+++ b/edit-post/components/visual-editor/index.js
@@ -29,7 +29,7 @@ function VisualEditor( { hasFixedToolbar, isLargeViewport } ) {
 			<WritingFlow>
 				<PostTitle />
 				<BlockList
-					showContextualToolbar={ isLargeViewport && ! hasFixedToolbar }
+					showContextualToolbar={ ! isLargeViewport || ! hasFixedToolbar }
 					renderBlockMenu={ ( { children, onClose } ) => (
 						<Fragment>
 							<BlockInspectorButton onClick={ onClose } />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, thanks

@youknowriad
Copy link
Contributor Author

youknowriad commented Mar 2, 2018

I need some clarification on the desired behavior here. For me, the current one in this PR is good but is this what we want? cc @jasmussen

@youknowriad
Copy link
Contributor Author

Ok so I guess the idea is to avoid the fixed toolbar entirely on mobile. It should be good now.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

@youknowriad youknowriad merged commit c47bdf5 into master Mar 2, 2018
@youknowriad youknowriad deleted the fix/mobile-toolbar branch March 2, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile Web Viewport sizes for mobile and tablet devices [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Mobile block toolbar is gone
3 participants