-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Site Profiler: Add MetricsSection component #90662
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
title={ translate( | ||
"Your site {{success}}performs well{{/success}}, but there's always room to be faster and smoother for your visitors.", | ||
{ | ||
components: { | ||
success: <span className="success" />, | ||
alert: <span className="alert" />, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How we will display the success and alert pieces are still to be discussed, but this is a way to show how they will appear on the section title.
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~816 bytes added 📈 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the components, and they look great. I have left a non-blocking comment that can be addressed in this or in a follow-up task
onCTAClick={ () => setIsGetReportFormOpen( true ) } | ||
/> | ||
<BasicMetrics ref={ basicMetricsRef } basicMetrics={ basicMetrics.basic } /> | ||
<MetricsSection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about creating a component to encapsulate all the display logic of advanced metrics? I was thinking in something like <AdvancedMetrics...
that will accept an array of refs (or just all the refs as different props) and the response of calling the /site-profiler/advance
endpoint.
That way, all the logic to iterate the response and decide what titles to show will be handled in a separate component that can grow independently of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a separate issue here: #90704
Related to https://github.com/Automattic/dotcom-forge/issues/6910
Proposed Changes
Add the MetricsSection component, this component will display a title, a name, and a link and render the children block inside of it.
Why are these changes being made?
This component will be used as the base of every section on the Site Profiler new version
Testing Instructions
/site-profiler/:url
. Ex:/site-profiler/example.com
Pre-merge Checklist