Closed rviscomi closed 2 years ago
@25prathamesh @rviscomi I've created a work in progress Draft PR with my suggestions. I've made as much progress as I can with the time I have available today, and I've listed the current bugs in the PR description. Any insight/help is greatly appreciated! Prathamesh, feel free to work from this branch if you agree with the direction - I will have some limited time tomorrow to continue working.
@tunetheweb same question as the other PR, could you merge this if I'm unavailable this week?
OK, but bit confused where this is. Are we merging @mel-ada 's changes into this one? And then good to merge if no further feedback? Or something else?
Yeah the plan is to finish the review on https://github.com/HTTPArchive/custom-metrics/pull/39 and merge into this. Then we can resolve the open feedback and merge this.
@tunetheweb @rviscomi @25prathamesh It looks like all metrics are working as expected - here is how I'm testing. If you agree please merge. Thanks!
"gaming_metrics":{
"detectUA-ChromeLH":true,
"detectUA-GTmetrix":true,
"detectUA-PageSpeed":true,
"imgAnimationStrict":true,
"imgAnimationSoft":true,
"fidIframeOverlayStrict":true,
"fidIframeOverlaySoft":true,
"lcpOverlayStrict":true,
"lcpOverlaySoft":true
}
Expected Result:
"gaming_metrics":{
"detectUA-ChromeLH":true,
"detectUA-GTmetrix":true,
"detectUA-PageSpeed":true,
"lcpOverlaySoft":false
}
CodePen:
https://cdpn.io/pen/debug/rNJpEQd?authentication_hash=NQMzYoKdVqLk (Note the debug
in the URL, this removes a CodePen iframe which was preventing inline UA detection from being picked up in @25prathamesh's comment here)
WPT Result: https://www.webpagetest.org/result/220531_AiDc2M_B2N/1/details/#waterfall_view_step1
Expected Result:
"gaming_metrics":{
"detectUA-ChromeLH":true,
"fidIframeOverlaySoft":false,
"lcpOverlaySoft":false
}
WPT Result: (See Example url in test, not a CodePen) https://www.webpagetest.org/result/220531_AiDcSX_91R/1/details/#waterfall_view_step1
Expected Result:
"gaming_metrics":{
"imgAnimationStrict":true,
"imgAnimationSoft":true,
"fidIframeOverlayStrict":true,
"fidIframeOverlaySoft":true,
"lcpOverlayStrict":true,
"lcpOverlaySoft":true
}
Test Codepen: https://cdpn.io/pen/debug/KKQQqwp?authentication_hash=dGkXWNjPjVdA
WPT result: https://webpagetest.httparchive.org/result/220531_1Y_2/1/details/#waterfall_view_step1
@25prathamesh will wait for your OK since you were going to check some other things. If I don’t hear back by this evening, then will go ahead and merge.
HI @tunetheweb
I have made a small changes to prevent lcpOverlaySoft:false passed on every test
commit ID - https://github.com/HTTPArchive/custom-metrics/commit/cca817ac2152513a941257e02a4a0be7707af897
I did another round of testing with new JS, and everything working properly, you can merge
Strict & Soft CWV Metric Hacks Passing CodePen Test URL - https://cdpn.io/pen/debug/KKQQqwp?authentication_hash=dGkXWNjPjVdA
WPT Test URL - https://www.webpagetest.org/result/220531_BiDcXB_BRX/
Inline JS UA Detection Passing CodePen Test URL - https://cdpn.io/pen/debug/eYVGjzr?authentication_hash=NjrYzpGOZPZA WPT Test URL - https://www.webpagetest.org/result/220531_BiDcFE_BS6/1/details/#waterfall_view_step1
External JS UA Detection Passing Test URL - https://lakmeindia.com/
WPT Test URL - https://www.webpagetest.org/result/220531_BiDcAE_BT9/1/details/#waterfall_view_step1
Looks like you made that commit on your own branch. Can you open a PR to this branch?
Looks like you made that commit on your own branch. Can you open a PR to this branch? just pushed to perf-hacks branch, refer below link https://github.com/HTTPArchive/custom-metrics/pull/37/commits/89e4309f312b768f281d670f675b0176afbf573b
Progress on #16 https://github.com/HTTPArchive/almanac.httparchive.org/issues/2890
Test page: https://codepen.io/rviscomi/pen/rNJpEQd Test results: https://webpagetest.httparchive.org/result/220528_ZS_E/1/details/#waterfall_view_step1