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

Editor: Hide plugin buttons in header on mobile layouts #49329

Merged
merged 1 commit into from
Jan 27, 2021

Conversation

p-jackson
Copy link
Member

Changes proposed in this Pull Request

  • Tidy up the icons in the header on mobile viewport widths.
  • Added a filter in case we want to disable this in a hurry.

This is a stripped down version of the original PR that also collapsed the publish/save/update buttons #49079

Testing instructions

  • Checkout branch, sandbox a test site, and run yarn dev --sync
  • Open a post or page in the block editor
  • Shrink viewport down to mobile size and observe the plugin buttons disappear (they should still be available in the more menu)

Fixes #48337

@p-jackson p-jackson added [Feature] Post/Page Editor The editor for editing posts and pages. Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin labels Jan 27, 2021
@p-jackson p-jackson requested a review from Copons January 27, 2021 04:09
@p-jackson p-jackson self-assigned this Jan 27, 2021
@matticbot
Copy link
Contributor

@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 27, 2021
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D56047-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@Copons
Copy link
Contributor

Copons commented Jan 27, 2021

This works well except for an edge case where pinned plugins might appear before the settings icon.
Given we are only keeping the first-child, it means that if a plugin icon is the first, we end up hiding the settings:

Screenshot 2021-01-27 at 11 31 42

Screenshot 2021-01-27 at 11 31 49

As far as I can see, the repro steps are a bit weird:

  • Click on the JP icon to open the JP sidebar.
  • Click on the star icon (only visible on large screens) to unpin JP from the header.
  • Click on the star icon again.
  • ⚠️ The JP icon is now before the settings.

The weird part is that this instead works appropriately:

  • Click on the JP icon to open the JP sidebar.
  • Click on the star icon to unpin JP from the header.
  • Close the JP sidebar.
  • Open the JP sidebar through the More menu.
  • Click on the star icon to pin JP to the header.
  • ✅ The JP icon is now after the settings.

In the scenario where the settings icon is hidden, AFAIK the only way of opening the post/block sidebar (aside from resizing the window or hit ⇧⌘,, which might not be possible on mobile) would be to select a block and click on "Show More Settings" (first item of the block toolbar's "more options" menu).


As I said at the beginning, I'm inclined to consider this an unlikely edge case, which has a clear (although not straightforward) workaround.

Also, as mentioned internally, Gutenberg itself should have hidden the icons on small screens in the first place, and this is apparently a regression.
With this in mind, the WPCOM fix should be considered temporary (hopefully, at least 😄), which IMHO makes this good enough even with the edge case issue.

Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

This LGTM, and making it filterable is an excellent idea! ✨

I'm "pre-approving" this as I deem the edge case is unlikely enough.
Although, before merging, I'd recommend to observe p7DVsv-amS-p2, because @jasmussen is working on a fix on Gutenberg, which might render this PR obsolete very quickly.

@jasmussen
Copy link
Member

I have a fix for this in core, here: WordPress/gutenberg#28521

This seems like a lot of code for the fix, but I'll defer to you on the details. But it would also be good to remove this again once the core fix lands.

@Copons
Copy link
Contributor

Copons commented Jan 27, 2021

The Core fix is scheduled for 9.9, which should land next week.
I guess we can wait 1-2 week for 9.9 to merge into WPCOM rather than have multiple ETK releases, considering that the issue is visually (very!) unpleasant, but not a blocker. 🤔
@simison do you have any preferences?

@p-jackson
Copy link
Member Author

@Copons I see that @simison has listed it as a blocker for the next onboarding test (pbAok1-1PR-p2) so I'm going to deploy this fix (it's not the only blocker of course, but I wouldn't want to be in the situation where we're waiting for a GB release before we start testing)

This seems like a lot of code for the fix

It is! Some of the code is unfortunately needed to satisfy the webpack build. The other code is so we can quickly disable it without an ETK release. Could be useful if when we ship GB 9.9 but aren't immediately in a position to ship a version of ETK.

edge case where pinned plugins might appear before the settings icon

I see the core fix has also WordPress/gutenberg#28526 dealt with this by just hiding everything after the first-child too.

@p-jackson p-jackson merged commit f684547 into trunk Jan 27, 2021
@p-jackson p-jackson deleted the update/collapse-plugin-icons-on-mobile branch January 27, 2021 20:31
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 27, 2021
@p-jackson p-jackson mentioned this pull request Jan 27, 2021
2 tasks
@Copons
Copy link
Contributor

Copons commented Jan 27, 2021

edge case where pinned plugins might appear before the settings icon

I see the core fix has also WordPress/gutenberg#28526 dealt with this by just hiding everything after the first-child too.

Yup!
It's an acceptable stopgap, but there will be a follow up in Gutenberg to either move the settings button outside the pinned items container, or add a specific CSS class to it.
We might want to keep observing the header for a while, and revert this change once the "final" Core fix has found its way to WPCOM.
Meanwhile, the current Core workarounds won't conflict with this, so there won't be any urgency in reverting this change eventually. 🙂

@jasmussen
Copy link
Member

Thanks for the work. I'm happy to be pinged directly for any followup questions or PRs that need to be done here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin [Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Launch button and plugin icons need menu collapse on mobile
4 participants