CLS diff not showing on compare screen #784
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
| if ( | ||
| group !== 'metrics' && // if metrics group audit is found, don't skip it |
There was a problem hiding this comment.
this changes the behavior that was intentional if both were passing, is there a specific case you have in mind for changing this?
There was a problem hiding this comment.
I think @patrickhulce we agreed here on skipping this check when the group id is metrics.
There was a problem hiding this comment.
Ah thanks for the reminder it had been awhile :) sg!
| // @ts-ignore | ||
| const thresholds = SCORE_LEVEL_METRIC_THRESHOLDS[numericDiff.auditId]; | ||
| if (thresholds) { | ||
| if (numericDiff.compareValue <= thresholds[0]) { |
There was a problem hiding this comment.
why are you doing this? the score already has its own thresholds and we want them to match the actual score.
There was a problem hiding this comment.
With current implementation, CLS audit always get score of 1 for me and because it is always 1, it always show passing icon for CLS audit diff. I may be wrong with my observation for having score 1 for CLS. Please correct me if my observation is wrong here.
There was a problem hiding this comment.
@patrickhulce I just tried again with a page which has bad CLS score and I can see non 1 score for CLS, which means my assumption was wrong there. Let me fix the changes accordingly and come back.
There was a problem hiding this comment.
@patrickhulce I have reverted the changes for this file as I have verified that scores are working fine and we don't need to handle the thresholds separately for CLS.
There was a problem hiding this comment.
thanks for the contribution @vikasrohit! just one more revert and I think we're good here
| @@ -58,7 +58,10 @@ const ChangedAuditList = props => { | |||
| onClick={() => props.setSelectedAuditId(audit.id || null)} | |||
| > | |||
| <div className="audit-group__audit-score"> | |||
| <ScoreIcon score={audit.score || 0} /> | |||
| <ScoreIcon | |||
There was a problem hiding this comment.
I think we can revert these too now :)
There was a problem hiding this comment.
Done. Thanks for noticing it, my bad I missed it with last revert.
|
thanks very much @vikasrohit! |
#689 CLS diff not showing on compare screen
Known issues in the PR:I have added duplicate declaration forSCORE_LEVEL_METRIC_THRESHOLDSinscore-icon.jsxbecause I was not sure what is the ideal way for having it at common place as I didn't see any constant or utils file for the server module. Please let me know, I can fix that quickly.Not able to resolve the linting error forconst thresholds = SCORE_LEVEL_METRIC_THRESHOLDS[numericDiff.auditId];inscore-icon.jsx#30