-
Notifications
You must be signed in to change notification settings - Fork 818
Boost: Surface Error details for LCP Optimization #43647
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
Conversation
…mization failures * Migrate LCP status display logic to a new Status component. * Add ErrorTooltip component to show detailed error messages for failed optimizations. * Update LCP state management to include error handling in the backend. * Introduce new SCSS styles for error tooltips and status display. * Ensure compatibility with existing LCP state management and API responses.
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
$black: #101517; | ||
$white: #fff; | ||
|
||
.jb-error-tooltip { |
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.
As we do not have many tooltips within the Boost Settings page, I took inspiration from the Boost Score Graph for the View Details tooltip.
https://github.com/Automattic/jetpack/blob/add/boost/lcp/error-messaging/projects/js-packages/components/components/boost-score-graph/tooltip.tsx#L8
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.
Does the IconTooltip component from RNA work for your use case? If so, try to use that.
We have a storybook configured for all the RNA components.
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.
Unfortuntely not precisely as the icon itself appears to be the anchor tag for that React component, where as I wanted the "View details" to be anchor point. I wanted to maintain the pattern of having an info icon to the left similarly to Critical CSS' Advanced Recommendations, but instead of the new page, it's just the tooltip.
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.
To me, "View details" implies that the link will open a page. The following conveys the same message, but more clearly IMHO.
2 pages could not be optimized. ℹ
However, this is a design problem. So, cc @ederrengifo
I also think it would be more consistent to use the notice component. Perhaps use the "warning" level instead of "error" to avoid unwarranted panics.
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 couple of prototypes to see how these might show:
Option 1: Similar to the existing styling that we have for Critical CSS, except when clicking "View details" the tooltip appears.
Option 1 Screenshots
Tooltip has a black background on white text for consistency with the Historical Performance tooltip:
Option 2: Icon Tooltip that when clicked, shows the errors/details.
Option 3: Notice component with Icon Tooltip that when clicked, shows the errors/details.
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.
Personally I like the notice component, it stands out a bit more on the page, as something that should be acted on.
That said, if we picked that we'd have to update the other notices to remain consistent.
Maybe for now we should remain consistent, and we can aim to update the design of all notices in a future PR?
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.
Code Coverage SummaryCoverage changed in 5 files.
2 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
* Add ErrorDetails and ErrorDetailsTooltip components for improved error display in LCP status. * Update status.module.scss to reflect changes in error handling structure.
…on checks * Introduce checks to determine if background images can be optimized before processing. * Refactor logic to skip elements that do not meet optimization criteria.
* Enhance the ErrorDetails component to include a summary wrapper for better layout. * Refactor SCSS styles for error tooltips and summary display, improving visual consistency. * Simplify the rendering logic in the Status component by directly integrating ErrorDetails.
* Integrate FoldingElement for better error message display in the ErrorDetails component. * Simplify error message rendering and enhance user interaction with expandable details. * Adjust SCSS styles for improved layout and visual consistency in error summaries.
…/jetpack into add/boost/lcp/error-messaging
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.
As I was mentioning during the call. I think this style is the way to go. My only concern is surfacing the internal errors publicly. Let's intercept the errors and show human readable versions of them instead. But, that's outside of the scope of this PR.
However, to effectively show the errors without exposing internal details we may have to change the schema of the errors in the LCP report to something like:
errors: <{
type: 'url_validation'|'http_error'|'unknown_type', // Any anticipated types of the error that helps us classify.
meta: ErrorMeta // It would have to adapt to the type. So, 'url_validation' would probably have the url as part of the ErrorMeta object.
}>[]
To be clear, I do not think the schema update needs to be a part of this PR.
if ( in_array( $lcp_data['element'], $selectors, true ) ) { | ||
// If we already printed the styling for this element, skip it. | ||
continue; | ||
} | ||
$selectors[] = $lcp_data['element']; | ||
|
||
$lcp_optimizer = new LCP_Optimization_Util( $lcp_data ); | ||
$image_url = $lcp_optimizer->get_image_to_preload(); | ||
$image_url = $lcp_optimizer->get_image_to_preload(); |
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 renamed this method in #43667
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 the heads up. I've made this PR dependent on #43667 now
Yep, I agree. I think communicating the errors, and suggesting potential fixes (dare I say recommendations) may be the way to go, but I have created HOG-53: Surface errors from LCP analysis in the UI to focus on this post Beta release. |
… add/boost/lcp/error-messaging
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.
LGTM!
Please remember to update the error schema before release.
Note
Merge this PR after #43667 is merged.
Fixes HOG-53: Surface errors from LCP analysis in the UI
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Prerequisites:
JETPACK_BOOST_ALPHA_FEATURES
)Happy Path
HTTP Error
Use the below snippet to enforce a HTTP non 200 for LCP Analysis:
Force non-happy path