-
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
Logged in Profiler: Allow searching pages in dropdown and show empty states #95415
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~127 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.
Functionality-wise, this PR works 👍
Added a comment where the copy might cause confusion.
I didn't realize it when reading the copy from the original issue description, but it felt a bit weird when testing.
if ( item.value === '-1' ) { | ||
return ( | ||
<div className="message"> | ||
{ translate( 'Searching your top 20 most popular pages.' ) } |
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.
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.
@matt-west curious what you think of the copy from a UX point of view 🙏
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.
Thanks for highlighting @candy02058912. I agree that we can improve on the current copy.
We're really talking about a limitation of the performance feature here rather than just the search.
Something like this could work, but it's a bit long.
Performance testing is available for the 20 most popular pages.
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.
cc: @autumnfjeld what's your take on the copy suggestions here?
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.
Performance testing is available for the 20 most popular pages.
If this fits on two lines, this is good.
Slightly shorter option if needed and if @matt-west approved
Performance tests limited to the 20 most popular pages.
Performance tests limited to the top 20 pages.
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.
Looks good @vykes-mac. Let's address the copy suggestion, but otherwise ship it!
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.
Working as expected, good job @vykes-mac!
One optional comment.
); | ||
} | ||
if ( item.value === '-2' ) { | ||
return <div className="message">{ translate( 'No pages found' ) }</div>; |
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.
Since we're looking for a
specific page when using the searching, should it be Page not found
instead?
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 think because a search query can return a list of pages matching and not only a specific one No pages found
works. I think the copy works as is. but we can change later if others have a strong opinion.
6c79eb4
to
1191805
Compare
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 |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/16921039 Some locales (Hebrew, Japanese) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Thank you @vykes-mac for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Relates to https://github.com/Automattic/dotcom-forge/issues/9372
Proposed Changes
Searching your top 20 most popular pages.
No pages found
item when no pages are foundWhy are these changes being made?
Testing Instructions
/sites/performance/site
Searching your top 20 most popular pages.
copy is show at the bottom of the listNo pages found
is shownPre-merge Checklist