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

Styles: Load the @wordpress/components stylesheet #45724

Merged
merged 3 commits into from
Sep 22, 2020

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Sep 17, 2020

We've started using @wordpress/components for some components here and there in Calypso, but there is a fundamental issue with that: we're not loading any @wordpress/components styles yet. The reason is that the styles are decoupled from the package itself, and they're usually loaded through either of the following methods:

  • Invoking wp_enqueue_style() directly or as a dependency of another enqueued stylesheet - for WP environments.
  • Including the built stylesheet file in your app - for JS apps and non WP environments (like Calypso).

So this PR is including the stylesheet directly. This isn't a great solution, because that way we're including the entire stylesheet and don't benefit from tree shaking for those styles. However, improving that may and likely will require architectural changes in the way that the component styles are defined and consumed in the Gutenberg project. And this is a task for another time.

Finally, by including this stylesheet, we have to verify all the instances that use @wordpress/components already look well.

Changes proposed in this Pull Request

  • Styles: Load the @wordpress/components stylesheet globally.
  • Styles: Remove the @wordpress/components stylesheet from EditorDeprecationDialog as it'll now be loaded globally above.

Testing instructions

  • Do a smoke testing around Calypso to verify components are looking unchanged.
  • Go to /read.
  • Enable the shouldRenderAppPromo prop inside ReaderSidebarPromo that lives at the bottom of the sidebar.
  • Verify it looks good, as shown on the "After" screenshot:

Before fixing the button height after loading the @wordpress/components stylesheet:

After fixing the button height after loading the @wordpress/components stylesheet:

  • Go through these steps and verify the "Resend it" link looks good.
  • Verify there are no other affected changes that can be seen/reproduced.

Notes

cc @allendav @Automattic/hydra - any idea how we could repro the "Woo DNA" flow so we could test the magic login step? This also appears to be using @wordpress/components. Thanks to @DanReyLop for the testing instructions.

@tyxla tyxla added [Type] Enhancement Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components labels Sep 17, 2020
@tyxla tyxla requested review from sarayourfriend and a team September 17, 2020 11:19
@tyxla tyxla self-assigned this Sep 17, 2020
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Sep 17, 2020

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

App Entrypoints (~2232 bytes removed 📉 [gzipped])

name                 parsed_size           gzip_size
entry-main                -129 B  (-0.0%)    -1321 B  (-0.4%)
entry-login                -41 B  (-0.0%)     -904 B  (-0.4%)
entry-gutenboarding        -24 B  (-0.0%)       -7 B  (-0.0%)

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

Sections (~27 bytes removed 📉 [gzipped])

name         parsed_size           gzip_size
post-editor       -145 B  (-0.0%)      -27 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.

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.

@DanReyLop
Copy link
Contributor

any idea how we could repro the "Woo DNA" flow so we could test the magic login step?

  • Start with a self-hosted site.
  • Add this code to functions.php or somewhere that's run on every request on the site: define( 'WOOCOMMERCE_CALYPSO_ENVIRONMENT', 'development' );
  • Run Calypso locally, as usual (on calypso.localhost:3000)
  • Install and activate the WooCommerce Payments plugin.
  • You should be automatically redirected, but in case you aren't, click on the [$] Payments menu on WP-Admin.
  • Click on the big "Set Up" button. You'll be redirected to the "Woo DNA" flow on Calypso.
  • If you're logged into WP.com already, click on "Create a new account or connect as a different user".

There, if you input the email of a passwordless account, you should get a Woo-branded "magic login" splash screen (and email).

I hope that works on the first try, if not, please ping me :)

@tyxla
Copy link
Member Author

tyxla commented Sep 17, 2020

Thanks a bunch, @DanReyLop, that did the job! 👍 💯

Copy link
Contributor

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

LGTM. App promo looks fine.

Good catch!

sarayourfriend added a commit that referenced this pull request Sep 17, 2020
They'll be imported globally by #45724
@tyxla tyxla force-pushed the add/wordpress-components-stylesheet branch from 9fc330a to e47e308 Compare September 18, 2020 07:24
@simison
Copy link
Member

simison commented Sep 18, 2020

I vaguely remember there was something funky about importing these component styles but I can't trace now what it was:

// @TODO: Remove this line and restore the import in `./gutenboard.tsx` when a release after @wordpress/components@8.5.0 is published.
// See https://github.com/WordPress/gutenberg/pull/19535
@import '~@wordpress/components/build-style/style';

WordPress/gutenberg#19535

@sirreal @ockham rings any bells?

@simison
Copy link
Member

simison commented Sep 18, 2020

BTW you can always import sass styles as well, but you'd have to make variables available with @wordpress/base-styles. Might help with reskinning colors to match with WordPress.com?

@tyxla tyxla force-pushed the add/wordpress-components-stylesheet branch from e47e308 to 067433c Compare September 18, 2020 17:18
@griffbrad
Copy link
Contributor

@jsnajdr @saramarcondes This might be something worth investigating more in Gutenberg. While wp-dmin will likely have to load this monolithic CSS file for the foreseeable future, we don’t really want to lose the ability to load styles per component in calypso.

@tyxla tyxla force-pushed the add/wordpress-components-stylesheet branch from 067433c to 31e2a87 Compare September 18, 2020 20:11
@tyxla tyxla force-pushed the add/wordpress-components-stylesheet branch from 31e2a87 to 1a37854 Compare September 18, 2020 20:50
@tyxla
Copy link
Member Author

tyxla commented Sep 21, 2020

@griffbrad while I agree this isn't an ideal approach, it is something that helps unblock us for now. We'd like to start using @wordpress/components in Calypso and that makes it possible for us to make progress.

I'd very much like to start investigating solutions for being more modular with these styles and even if we want to merge this now, we'll definitely consider ways to get there in the future, both here and in the Gutenberg core.

So, does anyone have any objections to merging this as-is? cc @Automattic/luna @Automattic/team-calypso

@simison
Copy link
Member

simison commented Sep 21, 2020

I'd very much like to start investigating solutions for being more modular with these styles and even if we want to merge this now, we'll definitely consider ways to get there in the future, both here and in the Gutenberg core.

@sgomes do I remember right that we've talked about this in context of Gutenboarding in the past? Were there some ideas written down after those convos?

So, does anyone have any objections to merging this as-is?

No objections, and would be great to have a clear road to better situation in near-term.

@sirreal
Copy link
Member

sirreal commented Sep 21, 2020

I haven't tested or reviewed this closely. No objections to the approach, I think a global fix like this makes sense at this time. Ideally, it will help continued adoption of these components.

@sgomes
Copy link
Contributor

sgomes commented Sep 21, 2020

@sgomes do I remember right that we've talked about this in context of Gutenboarding in the past? Were there some ideas written down after those convos?

@simison I don't remember any conversations around making Gutenberg styles more modular, sorry. But that could very well be the fault of my forgetful brain 😄

I do recall previous discussions with regards to build-time CSS-in-JS experiments; is that what you're referring to?

@tyxla
Copy link
Member Author

tyxla commented Sep 22, 2020

Thanks, folks! Shipping this for now, with the idea to explore a modular style import approach in mind.

@tyxla tyxla merged commit 9c2bc96 into master Sep 22, 2020
@tyxla tyxla deleted the add/wordpress-components-stylesheet branch September 22, 2020 07:57
@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 Sep 22, 2020
tyxla pushed a commit that referenced this pull request Sep 24, 2020
They'll be imported globally by #45724
tyxla added a commit that referenced this pull request Sep 25, 2020
* WIP: WordPress components gallery

* Update type definition depdency version

* Fix linting errors

* Fit name of new page on one line

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* Update client/devdocs/design/wordpress-components-gallery/autocomplete.scss

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>

* Make autocomplete match real-world examples

* Rename default exports to be more descriptive

* Remove importing of WP components styles

They'll be imported globally by #45724

Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants