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.

Sign up for GitHub

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

Jump to bottom

Jetpack Cloud: Add In Progress Scan Section #40212

Merged
merged 5 commits into from
Mar 19, 2020
Merged

Jetpack Cloud: Add In Progress Scan Section #40212

merged 5 commits into from
Mar 19, 2020

Conversation

enejb
Copy link
Member

@enejb enejb commented Mar 17, 2020

Changes proposed in this Pull Request

  • Adds a new section in Progress. This is just a dummy section.
    That shows the In Progress State.

See
Screen Shot 2020-03-17 at 2 07 57 PM

Testing instructions

  • Load jetpack cloud.

Fixes #

@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=add/scan-in-progress-component

@matticbot
Copy link
Contributor

matticbot commented Mar 17, 2020 •
edited
Loading

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

Sections (~221 bytes added 📈 [gzipped])

name           parsed_size           gzip_size
jetpack-cloud       +625 B  (+0.2%)     +221 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.

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.

@enejb enejb self-assigned this Mar 17, 2020
@enejb enejb requested a review from a team March 17, 2020 13:29
@enejb enejb added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Jetpack Jetpack Cloud Anything related to the Jetpack Cloud (cloud.jetpack.com) labels Mar 17, 2020
rcanepa
rcanepa approved these changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@rcanepa rcanepa left a comment

Choose a reason for hiding this comment

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

I'm approving this because the only issue I found is the one about not localizing two strings and I believe it was on purpose.

The icon is not exactly the same we have in our design, is that intentional as well?

client/landing/jetpack-cloud/sections/scan/main.jsx
<h1 className="scan__header scan__header--okay">{ translate( 'Preparing to scan' ) }</h1>
<ProgressBar value={ 1 } total={ 100 } color="#069E08" />
<p>
Welcome to Jetpack Scan, we are taking a first look at your site now and the results will
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional not to localize this string?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to skip the localization of this for now.

client/landing/jetpack-cloud/sections/scan/main.jsx
be with you soon.
</p>
<p>
We will send you an email once the scan completes, in the meantime feel free to continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am skipping translation here for now so that we can get more feedback on this text before they get translated.

rcanepa
rcanepa reviewed Mar 18, 2020
View reviewed changes
config/jetpack-cloud-development.json
@@ -16,7 +16,8 @@
"jetpack-cloud/backups-restore": true,
"jetpack-cloud/settings": true,
"jetpack-cloud/scan": true,
"jetpack-cloud/scan-history": true
"jetpack-cloud/scan-history": true,
"jetpack-cloud/on-demand-scan": true
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the idea behind this key?

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes the button Scan now button show up. So that it is easier to test.

@enejb
Copy link
Member Author

enejb commented Mar 18, 2020 •
edited
Loading

The icon is not exactly the same we have in our design, is that intentional as well?

Yes I didn't to spend time now implementing the animation part just yet.

@enejb enejb force-pushed the add/scan-in-progress-component branch from 0775948 to c1560db Compare March 18, 2020 15:20
@enejb enejb force-pushed the add/scan-in-progress-component branch from 804c58e to f393baa Compare March 19, 2020 08:00
@enejb enejb merged commit 5418e13 into master Mar 19, 2020
@enejb enejb deleted the add/scan-in-progress-component branch March 19, 2020 08:19
@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 Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcanepa rcanepa rcanepa approved these changes

Assignees

@enejb enejb

Labels
Jetpack Cloud Anything related to the Jetpack Cloud (cloud.jetpack.com) Jetpack
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@enejb @matticbot @rcanepa