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

Gutenboarding: Collapse launch button + plugin icons on mobile #49079

Conversation

StefanNieuwenhuis
Copy link
Contributor

@StefanNieuwenhuis StefanNieuwenhuis commented Jan 20, 2021

Changes proposed in this Pull Request

  • Tidy up the icons in the header on mobile viewport widths and collapse relevant actions.

Testing instructions

  • Checkout this branch
  • Ensure your site is sandboxed and you're logged in to your sandbox environment
  • Navigate to apps/editing-toolkit and run yarn dev --sync
  • Open a post or page on your chosen site to go to the editor
  • Shrink your viewport down to mobile size and observe the collapsed menu in the editor

Before
Screenshot 2021-01-22 at 10 38 46

After
Screenshot 2021-01-22 at 10 38 55

Video: https://www.loom.com/share/2709f1194d5a4e48b2fe154179089051

Fixes #48337

@matticbot
Copy link
Contributor

@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.

D55692-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

matticbot commented Jan 20, 2021

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime (~42 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
manifest        +95 B  (+0.3%)      +42 B  (+0.3%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

App Entrypoints (~80 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-main                  +398 B  (+0.0%)      +80 B  (+0.0%)
entry-login                 +105 B  (+0.0%)      +26 B  (+0.0%)
entry-gutenboarding         +105 B  (+0.0%)      +26 B  (+0.0%)
entry-domains-landing       +105 B  (+0.0%)      +26 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~3827 bytes removed 📉 [gzipped])

name                parsed_size           gzip_size
jetpack-connect          +255 B  (+0.0%)     +275 B  (+0.1%)
google-my-business       +183 B  (+0.1%)    +1027 B  (+1.2%)
marketing                -126 B  (-0.0%)     +614 B  (+0.4%)
plans                    +103 B  (+0.0%)      +17 B  (+0.0%)
devdocs                   -31 B  (-0.0%)       -4 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~44 bytes removed 📉 [gzipped])

name                                            parsed_size           gzip_size
async-load-design-wordpress-components-gallery        -31 B  (-0.0%)      -44 B  (-0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@simison simison changed the title Gutenboarding: Collapse launch button + plugin icons om mobile Gutenboarding: Collapse launch button + plugin icons on mobile Jan 20, 2021
@tjcafferkey tjcafferkey added the Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin label Jan 22, 2021
@@ -126,7 +129,7 @@
"@wordpress/html-entities": "*",
"@wordpress/i18n": "*",
"@wordpress/icons": "*",
"@wordpress/interface": "^0.7.5",
"@wordpress/interface": "^1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to double check this package upgrade wont negatively impact other features that are using it.

Copy link
Member

Choose a reason for hiding this comment

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

I think there recently was packages upgrade so rebasing might sort it.

Copy link
Contributor

@Copons Copons Jan 25, 2021

Choose a reason for hiding this comment

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

@wordpress/interface was replaced by @wordpress/edit-site in the toolkit in #48729, so I'd say that upgrading to 1.0.0 shouldn't cause any regressions.

</Button>
) }
renderContent={ () => (
<div className="dropdown-content">
Copy link
Contributor

Choose a reason for hiding this comment

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

These components may require additional props which we will also need to look into. You can find their usages here in Gutenberg

@Copons
Copy link
Contributor

Copons commented Jan 25, 2021

I'm personally very wary of introducing new .com-only UI patterns, especially considering that this issue is as valid on .com as it is on .org — maybe even more, as .org users might be more inclined to install dozens of plugins adding their icons to the header.
If we really want to go ahead with a mobile-only dropdown replacing plugins and other header actions, I think we should propose it on the Gutenberg repo instead (assuming it's not something being already worked on).

I also fear that this approach might prove to be too complicated to maintain, given the amount of variations the editor header can have. What about draft posts? What about the non-post editors?

As a quick stop-gap, we might want to start by simply hiding the plugins icons, which are still accessible through the More menu.

Fixing the header actions (switch to draft, preview, update/publish) is more difficult, as they don't exist anywhere else, and aren't semantically strictly related.
We also plug into the Preview button on .com to use the Calypso preview component. I haven't checked so it might work fine, but it's yet another example of things that could go wrong. 😄


tl;dr: why not just hide the plugin icons for now?

Comment on lines +38 to +44
wp_localize_script(
'a8c-fse-editor-header-buttons-script',
'wpcomEditorSiteLaunch',
array(
'locale' => determine_locale(),
)
);
Copy link
Member

@p-jackson p-jackson Jan 26, 2021

Choose a reason for hiding this comment

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

This looks like it was copy/pasted from another php file, but we don't need it here. This code adds a window.wpcomEditorSiteLaunch.locale global, but that global is not used by the editor header buttons.

We should delete these lines.

@p-jackson
Copy link
Member

The iPhone 8 layout works with just the plugin icons hidden

Screenshot 2021-01-27 at 11 39 48 AM

tl;dr: why not just hide the plugin icons for now?

I agree with this. Hiding the plugin icons fixes the layout issue and doesn't get us into the sticky situation of moving DOM items around. This has been flakey in the past and caused crashes in the past (if we manually update the DOM then React can break if it ever tries to re-render the header). Also sometimes the launch button is shown in the header and we'd need to duplicate all the logic for that too.

@p-jackson
Copy link
Member

If we do just hide the plugin buttons then I think all this PR needs is this bit:

@import '~@wordpress/base-styles/mixins';
@import '~@wordpress/base-styles/variables';
@import '~@wordpress/base-styles/breakpoints';

.interface-pinned-items > button:not(:first-child) {
	@media ( max-width: $break-medium ) {
		display: none;
	}
}

I'll post about this to make sure happiness are aware of what's coming up.

@p-jackson p-jackson force-pushed the update/gutenboarding/collapsible-settings-icons-on-mobile branch from 58df346 to e321d43 Compare January 26, 2021 23:29
@p-jackson
Copy link
Member

I've created another PR that fixes the issue by only hiding the plugin icons #49329 @Copons. There's nothing wrong with this PR (in fact I copy/pasted most of the code 😄) but I felt I was going to delete most of it anyway, and I thought we may still want to be able to easily refer to these changes.

@p-jackson
Copy link
Member

A temporary fix has been merged in #49329 instead.

There's also a fix for the underlying problem in WordPress/gutenberg#28521 and WordPress/gutenberg#28526 that'll be shipped in GB 9.9

@p-jackson p-jackson closed this Jan 27, 2021
@p-jackson p-jackson deleted the update/gutenboarding/collapsible-settings-icons-on-mobile branch January 27, 2021 20:35
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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