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

Framework: Introduce calypso-ui #33154

Merged
merged 18 commits into from
Jun 19, 2019
Merged

Conversation

ockham
Copy link
Contributor

@ockham ockham commented May 20, 2019

Changes proposed in this Pull Request

Create a calypso-ui package, and move ScreenReaderText and ProgressBar components there.

Alternative to @blowery's #32869 from which I've stolen pretty much all of the business logic. The only difference is that ScreenReaderText and ProgressBar are simpler components than Dialog and RootChild and thus easier to untangle.

Testing instructions

npm run distclean && npm ci
npm start

Does Calypso build and run?

@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components Packages labels May 20, 2019
@ockham ockham requested review from sirreal and a team May 20, 2019 11:01
@ockham ockham self-assigned this May 20, 2019
@matticbot
Copy link
Contributor

@ockham
Copy link
Contributor Author

ockham commented May 21, 2019

Tests are failing since Jest gets confused by import './style.scss'. Need to tell it to ignore or mock those files.

@ockham
Copy link
Contributor Author

ockham commented May 22, 2019

Relevant: Automattic/newspack-plugin#48 😬

@matticbot
Copy link
Contributor

matticbot commented May 27, 2019

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

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

name        parsed_size           gzip_size
entry-main      +3875 B  (+0.2%)     +928 B  (+0.2%)

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

Sections (~4895 bytes removed 📉 [gzipped])

name                  parsed_size           gzip_size
woocommerce               -1460 B  (-0.1%)     -327 B  (-0.1%)
post-editor               -1460 B  (-0.1%)     -323 B  (-0.1%)
plans                     -1460 B  (-0.3%)     -358 B  (-0.3%)
media                     -1460 B  (-0.4%)     -331 B  (-0.4%)
gutenberg-editor          -1460 B  (-0.2%)     -331 B  (-0.2%)
themes                    -1459 B  (-0.4%)     -320 B  (-0.3%)
stats                     -1459 B  (-0.1%)     -320 B  (-0.2%)
settings-performance      -1459 B  (-0.8%)     -387 B  (-0.8%)
settings                  -1459 B  (-0.2%)     -389 B  (-0.3%)
plugins                   -1459 B  (-0.3%)     -390 B  (-0.3%)
import                    -1459 B  (-0.7%)     -368 B  (-0.7%)
domains                   -1459 B  (-0.2%)     -372 B  (-0.2%)
checkout                  -1459 B  (-0.2%)     -359 B  (-0.2%)
activity                  -1459 B  (-0.2%)     -320 B  (-0.3%)

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 (~1270 bytes removed 📉 [gzipped])

name                                                         parsed_size           gzip_size
async-load-design-blocks                                         -1462 B  (-0.1%)     -307 B  (-0.1%)
async-load-post-editor-media-modal                               -1460 B  (-0.5%)     -331 B  (-0.4%)
async-load-my-sites-checklist-wpcom-checklist-component-jsx      -1460 B  (-1.1%)     -370 B  (-1.2%)
async-load-signup-steps-domains                                  -1459 B  (-0.7%)     -372 B  (-0.7%)
async-load-design-playground                                     -1435 B  (-0.1%)      +58 B  (+0.0%)
async-load-design                                                -1435 B  (-0.1%)      +52 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.

@justinshreve
Copy link
Contributor

As a follow-up to your note here about aligning Automattic/newspack-plugin#72 (comment), I was looking for some thoughts/feedback...

As part of the new WooCommerce Onboarding wizard, users are redirected into a special branded WooCommerce / Jetpack Connect flow that matches the rest of the core setup wizard. This includes using Muriel styled components throughout the experience. More specifically, the version of TextControl that aligns the label inside the input is used, which requires a bit more than just CSS to make work.

Screen Shot 2019-05-28 at 10 36 02 AM

We are using the first version of the Newspack components in WooCommerce Admin to avoid duplicating work which is great. We use a few different components from their package (checkbox, select, text input).

However, it feels a bit odd to add Newspack as a dependency to Calypso, just for use in our special version of the sign-up flow. We also only need the single TextControl component on the Calypso side of this.

tl;dr: Should we consider porting that version of TextControl as another early calypso-ui component? I'm happy to take a look at this after our meetup next week if so. If not, are we OK pulling those components in temporarily? Or should I just do the overrides I need in the context of our code with a note to migrate in the future?

@ockham ockham force-pushed the try/calypso-ui-with-progress-bar branch from f0aac15 to 8d2673a Compare May 29, 2019 09:02
@ockham
Copy link
Contributor Author

ockham commented May 29, 2019

Rebased

@ockham
Copy link
Contributor Author

ockham commented May 29, 2019

As a follow-up to your note here about aligning Automattic/newspack-plugin#72 (comment), I was looking for some thoughts/feedback...

As part of the new WooCommerce Onboarding wizard, users are redirected into a special branded WooCommerce / Jetpack Connect flow that matches the rest of the core setup wizard. This includes using Muriel styled components throughout the experience. More specifically, the version of TextControl that aligns the label inside the input is used, which requires a bit more than just CSS to make work.

Screen Shot 2019-05-28 at 10 36 02 AM

We are using the first version of the Newspack components in WooCommerce Admin to avoid duplicating work which is great. We use a few different components from their package (checkbox, select, text input).

However, it feels a bit odd to add Newspack as a dependency to Calypso, just for use in our special version of the sign-up flow. We also only need the single TextControl component on the Calypso side of this.

tl;dr: Should we consider porting that version of TextControl as another early calypso-ui component? I'm happy to take a look at this after our meetup next week if so. If not, are we OK pulling those components in temporarily? Or should I just do the overrides I need in the context of our code with a note to migrate in the future?

Thanks a lot for getting in touch, @justinshreve!

As per p1559073709014500-slack-components, it'd be great to migrate that TextControl into Calypso (and thus, calypso-ui) 🙂

@ockham ockham force-pushed the try/calypso-ui-with-progress-bar branch from 8d2673a to 2f88de9 Compare May 29, 2019 11:35
@blowery
Copy link
Contributor

blowery commented May 29, 2019

Looks like we have some false positives from the linter for dev dependencies and a couple existing a11y problems. Thoughts?

@sirreal
Copy link
Member

sirreal commented Jun 3, 2019

Looks like we have some false positives from the linter for dev dependencies

I really think we need to fork or contribute to the lint rule to better handle monorepos:

Until support is there, this shouldn't be a blocker. Lint errors serve as a reg flag for contributors and reviewers to verify dependencies are correctly declared. We can approve/comment noting the lint error is incorrect, and proceed.

a couple existing a11y problems

If they're not too involved, would be nice to get them sorted out before sharing the components.

@ockham ockham force-pushed the try/calypso-ui-with-progress-bar branch from 2f88de9 to fa3b22b Compare June 4, 2019 17:49
@ockham
Copy link
Contributor Author

ockham commented Jun 4, 2019

Looks like we have some false positives from the linter for dev dependencies

I really think we need to fork or contribute to the lint rule to better handle monorepos:

Until support is there, this shouldn't be a blocker. Lint errors serve as a reg flag for contributors and reviewers to verify dependencies are correctly declared. We can approve/comment noting the lint error is incorrect, and proceed.

👍

a couple existing a11y problems

If they're not too involved, would be nice to get them sorted out before sharing the components.

1eac061

Ready for another look!

@ockham ockham force-pushed the try/calypso-ui-with-progress-bar branch from fe18aed to a450b95 Compare June 6, 2019 08:15
@blowery blowery force-pushed the try/calypso-ui-with-progress-bar branch from a450b95 to a9d808e Compare June 11, 2019 19:28

rcopy( inputDir, outputDirEsm, copyOptions )
.then( results => {
console.log( 'copied %d files', results.length );
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we can drop all the logging here. it gets pretty noisy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed:

image

Looks like rcopy is noisy enough on its own:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disabling rcopy's debug flag makes more of a difference:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor

@blowery blowery left a comment

Choose a reason for hiding this comment

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

👍

@blowery blowery changed the title Try: Package ScreenReaderText and ProgressBar components and styles Framework: Introduce calypso-ui Jun 19, 2019
@blowery blowery merged commit 26b00a5 into master Jun 19, 2019
@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 Jun 19, 2019
@lancewillett lancewillett deleted the try/calypso-ui-with-progress-bar branch March 12, 2020 20:57
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

5 participants