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

SegmentedControl: migrate CSS to webpack #35051

Merged
merged 8 commits into from
Aug 17, 2019
Merged

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Aug 1, 2019

Migrates SegmentedControl CSS to webpack and also improves the DX of the component:

First, the SimplifiedSegmentedControl is re-implemented in terms of SegmentedControl and SegmentedControlItem. No duplicate code (like the HTML markup).

Second, make SegmentedControlItem a property of the parent component: SegmentedControl.Item. They are always used together and the DX is better:

Before:

import SegmentedControl from 'components/segmented-control';
import SegmentedControlItem from 'components/segmented-control/item';

return (
  <SegmentedControl>
    <SegmentedControlItem />
  </SegmentedControl>
);

After:

import SegmentedControl from 'components/segmented-control';

return (
  <SegmentedControl>
    <SegmentedControl.Item />
  </SegmentedControl>
);

We need just one import statement.

How to test:
Verify the various usages of the component. Used at a lot of places, but the refactoring is very straightforward and low-risk.

@jsnajdr jsnajdr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components [Status] Needs e2e Testing CSS Migration labels Aug 1, 2019
@jsnajdr jsnajdr requested a review from a team August 1, 2019 10:40
@jsnajdr jsnajdr requested review from a team as code owners August 1, 2019 10:40
@jsnajdr jsnajdr self-assigned this Aug 1, 2019
@jsnajdr jsnajdr added this to In progress in CSS: Migrate Styles to Module Imports via automation Aug 1, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Aug 1, 2019

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

Webpack Runtime (~8 bytes removed 📉 [gzipped])

name      parsed_size           gzip_size
manifest       +568 B  (+0.3%)       -8 B  (-0.0%)

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.

Legacy SCSS Stylesheet (~328 bytes removed 📉 [gzipped])

name       parsed_size           gzip_size
style.css      -2674 B  (-2.2%)     -328 B  (-1.6%)

The monolithic CSS stylesheet that is downloaded on every app load.
👍 Thanks for making the stylesheet smaller in this PR!

Sections (~272 bytes added 📈 [gzipped])

name                parsed_size           gzip_size
woocommerce              -626 B  (-0.0%)      +49 B  (+0.0%)
stats                    -579 B  (-0.1%)      +35 B  (+0.0%)
post-editor              -437 B  (-0.0%)      -36 B  (-0.0%)
reader                   +384 B  (+0.1%)      +15 B  (+0.0%)
posts-pages              -367 B  (-0.1%)      -18 B  (-0.0%)
posts-custom             -367 B  (-0.1%)      -18 B  (-0.0%)
gutenberg-editor         -348 B  (-0.1%)       +3 B  (+0.0%)
comments                 -252 B  (-0.0%)       -3 B  (-0.0%)
themes                   +244 B  (+0.1%)     +100 B  (+0.1%)
domains                  +244 B  (+0.0%)     +112 B  (+0.1%)
help                     -214 B  (-0.0%)       +4 B  (+0.0%)
google-my-business       -212 B  (-0.1%)       -4 B  (-0.0%)
media                    -209 B  (-0.1%)      +23 B  (+0.0%)
plans                    -199 B  (-0.0%)       +5 B  (+0.0%)
jetpack-connect          -199 B  (-0.0%)       +5 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 (~1798 bytes added 📈 [gzipped])

name                                                         parsed_size           gzip_size
async-load-design-playground                                      -692 B  (-0.0%)      -83 B  (-0.0%)
async-load-design                                                 -692 B  (-0.0%)      -13 B  (-0.0%)
async-load-design-blocks                                          -478 B  (-0.0%)      -25 B  (-0.0%)
async-load-post-editor-media-modal                                -303 B  (-0.1%)       +7 B  (+0.0%)
async-load-blocks-inline-help-popover                             -214 B  (-0.1%)       +4 B  (+0.0%)
async-load-signup-steps-plans                                     -199 B  (-0.1%)       +5 B  (+0.0%)
async-load-blocks-reader-full-post                                -193 B  (-0.4%)       +0 B
async-load-signup-steps-about                                     -192 B  (-0.4%)       +8 B  (+0.1%)
async-load-reader-site-stream                                     -152 B  (-0.5%)     +204 B  (+2.7%)
async-load-reader-feed-stream                                     -152 B  (-0.5%)     +216 B  (+2.8%)
async-load-reader-search-stream                                    -78 B  (-0.1%)     +742 B  (+2.5%)
async-load-extensions-woocommerce-app-store-stats-referrers        -49 B  (-0.1%)       -3 B  (-0.0%)
async-load-extensions-woocommerce-app-store-stats-listview         -49 B  (-0.3%)       -6 B  (-0.1%)
async-load-extensions-woocommerce-app-store-stats                  -49 B  (-0.1%)       -3 B  (-0.0%)
async-load-blocks-calendar-popover                                 -44 B  (-0.0%)       -6 B  (-0.0%)
async-load-reader-following-manage                                 -32 B  (-0.0%)     +751 B  (+2.1%)

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.

@jsnajdr jsnajdr force-pushed the migrate/segmented-control-css branch from 7d64a42 to 90d31c8 Compare August 2, 2019 10:55
@bluefuton bluefuton force-pushed the migrate/segmented-control-css branch 2 times, most recently from 91888e5 to 1640639 Compare August 11, 2019 23:53
@jsnajdr jsnajdr force-pushed the migrate/segmented-control-css branch from 1640639 to ead1370 Compare August 16, 2019 09:06
jsnajdr and others added 7 commits August 16, 2019 14:00
Removes code duplication and makes CSS modularization easier: styles for the
control and the item are used at exactly one place.
Removes the `onSelect` prop (it's present only on the simplified variant) and adds
the missing `primary` prop.
`SegmentedControl` and `SegmentedControl.Item` are always used together, so let's
merge them into one module and make all imports one-liners. Improves the DX and
makes CSS modularization easier.
@jsnajdr jsnajdr force-pushed the migrate/segmented-control-css branch from 4563110 to c7eb716 Compare August 16, 2019 12:02
Copy link
Contributor

@flootr flootr left a comment

Choose a reason for hiding this comment

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

I tested various usages and they all seem to work fine! Code changes look good 👍

CSS: Migrate Styles to Module Imports automation moved this from In progress to Approved Aug 16, 2019
@diegohaz
Copy link
Contributor

Just linking the discussion on the other PR as that relates to these changes as well: #35086 (comment)

Copy link
Contributor

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

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

Resolved conflicts with master, and e2e tests started failing. But they seem flacky and unrelated to this change.

Code looks good. I tested a few screens with this component and everything seems to work.

@jsnajdr jsnajdr merged commit 9e445e1 into master Aug 17, 2019
CSS: Migrate Styles to Module Imports automation moved this from Approved to Done Aug 17, 2019
@jsnajdr jsnajdr deleted the migrate/segmented-control-css branch August 17, 2019 11:41
@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 17, 2019

Resolved conflicts with master

Thanks for the review @diegohaz! I usually prefer rebasing the branch rather than merging master into it. The commit history is then nicer and easier to work with.

@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 Aug 17, 2019
@simison
Copy link
Member

simison commented Aug 20, 2019

I'm not entirely sure what's going on but I see this on Jetpack plans grid (https://wordpress.com/plans + choose a Jetpack site):

Screenshot 2019-08-20 at 10 06 28

I see it only occasionally, like every 5th refresh. When it's broken, I see the CSS that I have in the screenshot.

I suspect this PR changed this but I haven't tested yet to confirm.

When it works, I see this instead:

image

Note margin: 0 which seems to cause the trouble.

Occurs in all screen sizes both on Firefox and Chrome.

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 20, 2019

Thanks for noticing and reporting @simison! Fix is ready for review in #35607.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants