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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jetpack Cloud: Scanner display threats #41216

Merged
merged 10 commits into from
Apr 20, 2020
Merged

Conversation

enejb
Copy link
Member

@enejb enejb commented Apr 17, 2020

Changes proposed in this Pull Request

This PR attempts to render the treats as best as it can.
We should move most of the treats details to the api vs figuring here.
I reused quite a bit of code from treats-alert.jsx component to figure out nicer nameing etc.

After:
Screen Shot 2020-04-17 at 12 04 37 PM

Bonus part here is that treat ignoring works as expected 馃コ for the treat that I ignored.

Testing instructions

Notice the sections renders real threats.

@enejb enejb added Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Jetpack Cloud Anything related to the Jetpack Cloud (cloud.jetpack.com) labels Apr 17, 2020
@enejb enejb requested a review from a team April 17, 2020 10:08
@matticbot
Copy link
Contributor

} );

case 'database':
return translate( 'Database %(threatCount)d threat', 'Database %(threatCount)d threats', {
Copy link

Choose a reason for hiding this comment

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

鈩癸笍 String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 17 times:
translate( 'Jetpack identified %(threatCount)d threat in your database.', 'Jetpack identified %(threatCount)d threats in your database.' ) ES Score: 7

@matticbot
Copy link
Contributor

matticbot commented Apr 17, 2020

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

Webpack Runtime (~20 bytes added 馃搱 [gzipped])

name      parsed_size           gzip_size
manifest        +27 B  (+0.0%)      +20 B  (+0.1%)

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 (~17 bytes added 馃搱 [gzipped])

name        parsed_size           gzip_size
entry-main       +126 B  (+0.0%)      +17 B  (+0.0%)

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

Sections (~7485 bytes added 馃搱 [gzipped])

name  parsed_size            gzip_size
scan     +21585 B  (+13.5%)    +7485 B  (+17.9%)

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.

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.

The UI breaks a bit when going from a very large screen to smaller one.

Kapture 2020-04-17 at 10 50 36

I now exactly how to fix that. Do you want me to provide a quick fix?

@enejb enejb requested a review from a team April 17, 2020 14:59
@ChaosExAnima ChaosExAnima self-assigned this Apr 17, 2020
@ChaosExAnima ChaosExAnima added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 17, 2020
@ChaosExAnima
Copy link
Contributor

Taking this over. Going to get typing updated and test with a bunch of threats.

@ChaosExAnima ChaosExAnima added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 17, 2020
@enejb
Copy link
Member Author

enejb commented Apr 20, 2020

This is working really great! Thanks for all the fixed @ChaosExAnima !

@rcanepa
Copy link
Contributor

rcanepa commented Apr 20, 2020

I just pushed a small update that changes the positioning of the buttons on mobile.
image

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.

Works great!

@enejb enejb merged commit bf2e29a into master Apr 20, 2020
@enejb enejb deleted the add/threats-to-jetpack-cloud branch April 20, 2020 15:05
@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 Apr 20, 2020
@a8ci18n
Copy link

a8ci18n commented May 15, 2020

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/3324647

Thank you @enejb for including a screenshot in the description! This is really helpful for our translators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Jetpack Cloud Anything related to the Jetpack Cloud (cloud.jetpack.com) Jetpack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants