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

Masterbar: display a Hamburger menu on mobile #12299

Merged
merged 4 commits into from
May 8, 2019

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented May 8, 2019

Follow-up from #11766

Changes proposed in this Pull Request:

This Hamburger menu is necessary so folks can access the wp-admin navigation on mobile.

Without it, when on mobile, one has no way to go from one wp-admin page to another:

image

Testing instructions:

  • Connect Jetpack to WordPress.com on a new site, and enable the WordPress.com Toolbar feature under Jetpack > Settings > Writing
  • On Mobile, go to the main wp-admin page, /wp-admin/
  • Attempt to go to Tools > Site Health => You can't.
  • Apply patch.
  • You now have a Hamburger menu you can use.
    image

Proposed changelog entry for your changes:

  • WordPress.com Toolbar: restore navigation hamburger on mobile.

Follow-up from #11766

This Hamburger menu is necessary so folks can access the wp-admin navigation on mobile.
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Pri] High [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations labels May 8, 2019
@jeherve jeherve added this to the 7.3.1 milestone May 8, 2019
@jeherve jeherve requested review from jmdodd and davemart-in May 8, 2019 12:52
@jeherve jeherve requested a review from a team as a code owner May 8, 2019 12:52
@jeherve jeherve self-assigned this May 8, 2019
@jetpackbot
Copy link

jetpackbot commented May 8, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 3, 2019.
Scheduled code freeze: May 27, 2019

Generated by 🚫 dangerJS against 53e1b85

We use wp_is_mobile() here because our own jetpack_is_mobile() would return true even on tabletss, where the wp-admin navigation is toggled:
https://github.com/WordPress/WordPress/blob/f5cab6780f67195bf717eb33f2a3661ef5f95242/wp-admin/js/common.js#L512
dereksmart
dereksmart previously approved these changes May 8, 2019
Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

WFM!

That hamburger is hidden via CSS on non-mobile views.
Relying only on CSS allows us to cover all edge cases, like folks on desktop but with a very small viewport that would trigger the wp-admin navigation menu to be toggled.

Related discussion: p1557322468103000-slack-jetpack-plugin
Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

👍 even better!

@jeherve jeherve removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label May 8, 2019
@jeherve jeherve merged commit fc9811b into master May 8, 2019
@jeherve jeherve deleted the fix/masterbar-wp-admin-nav-mobile branch May 8, 2019 14:04
jeherve added a commit that referenced this pull request May 8, 2019
* Masterbar: display a Hamburger menu on mobile

Follow-up from #11766

This Hamburger menu is necessary so folks can access the wp-admin navigation on mobile.

* Masterbar: only display the Hamburger menu on mobile

We use wp_is_mobile() here because our own jetpack_is_mobile() would return true even on tabletss, where the wp-admin navigation is toggled:
https://github.com/WordPress/WordPress/blob/f5cab6780f67195bf717eb33f2a3661ef5f95242/wp-admin/js/common.js#L512

* Always add the hamburger menu toggle.

That hamburger is hidden via CSS on non-mobile views.
Relying only on CSS allows us to cover all edge cases, like folks on desktop but with a very small viewport that would trigger the wp-admin navigation menu to be toggled.

Related discussion: p1557322468103000-slack-jetpack-plugin

* Masterbar: rely on core function to add toggle.
@jeherve
Copy link
Member Author

jeherve commented May 8, 2019

  • Cherry-picked to branch-7.3 in c6bf1c2
  • Cherry-picked to branch-7.3-built in b3f09c4

Internal reference: p9o2xV-k5-p2

jeherve added a commit that referenced this pull request May 8, 2019
* Masterbar: display a Hamburger menu on mobile

Follow-up from #11766

This Hamburger menu is necessary so folks can access the wp-admin navigation on mobile.

* Masterbar: only display the Hamburger menu on mobile

We use wp_is_mobile() here because our own jetpack_is_mobile() would return true even on tabletss, where the wp-admin navigation is toggled:
https://github.com/WordPress/WordPress/blob/f5cab6780f67195bf717eb33f2a3661ef5f95242/wp-admin/js/common.js#L512

* Always add the hamburger menu toggle.

That hamburger is hidden via CSS on non-mobile views.
Relying only on CSS allows us to cover all edge cases, like folks on desktop but with a very small viewport that would trigger the wp-admin navigation menu to be toggled.

Related discussion: p1557322468103000-slack-jetpack-plugin

* Masterbar: rely on core function to add toggle.
@mlaetitia
Copy link

Reported here: 2016105-zd

@matticbot matticbot added the Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". label May 9, 2019
@simison
Copy link
Member

simison commented May 9, 2019

on wpcom wp-admin, Gutenberg's post-publish view looks like this now:

image

Editor itself:

image

@jeherve
Copy link
Member Author

jeherve commented May 9, 2019

Yep, same in Jetpack. This isn't new with this PR though, it used to be like that with the Masterbar as well. Took note of it in #12320

kraftbj added a commit that referenced this pull request May 14, 2019
kraftbj added a commit that referenced this pull request May 23, 2019
kraftbj added a commit that referenced this pull request May 23, 2019
* Revert "Masterbar: display a Hamburger menu on mobile (#12299)"

This reverts commit fc9811b.

* Revert of f6e7433

* Fix remaining PHPCS warnings
kraftbj added a commit that referenced this pull request May 23, 2019
* Revert "Masterbar: display a Hamburger menu on mobile (#12299)"

This reverts commit fc9811b.

* Revert of f6e7433

* Fix remaining PHPCS warnings
kraftbj added a commit that referenced this pull request May 23, 2019
* Revert "Masterbar: display a Hamburger menu on mobile (#12299)"

This reverts commit fc9811b.

* Revert of f6e7433

* Fix remaining PHPCS warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants