-
Notifications
You must be signed in to change notification settings - Fork 108
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
Upgrade web-vitals to 3.3.1 and switch to attribution build #115
Conversation
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 like the changes (especially more sharing with web-vitals lib, nice), but I think we could fiddle with the settings a bit to change the permutations.
See comments inline.
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.
Cool, LTGTM.
Several optional follow-ups but no blockers.
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.
Nice find!
Doing some local testing and noticed the console logs that print a table-view do not use [Web Vitals Extension] prefix. I don't know if there is a clean way to do that using console.table command, but, perhaps you could use console.group('[Web Vitals Extension]') and console.groupEnd() to wrap the table? |
LGTM, I really liked the Table view for both LCP, CLS and INP breakdowns. Thanks for reducing the digits a bit on CLS and adding the "[Web Vitals Extension]" label in the Table. |
(Barry: Im working on a small patch to try to console.group changes, will send over) One more thing, once I tried to open the setting page and it just stalled on me. I noticed I have errors in the extension page and one is related to manifest permissions and the options page: ...wonder if its related to manifest v3, but, I can open the options page most of the time fine, so no idea... |
...it just happened again. I am not sure, but it may depend on the specific tab/page I have opened when I select the options menu item. I don't see why it should matter, but maybe the handler for the extension contextmenu is trying to access the current page? |
We only get a shift score per set of shifts. For example if you add a banner image, and it shifts three images down then that's one shift, one shift score, and three elements affected. It would be SOOO much nicer if we got the element causing the shift rather than the victims :-( Makes a bit more sense when there's two sets of shifts: An alternative would be this, but don't like it myself: WDYT? |
That seems fine to leave as-is and to follow-up as needed. (FWIW even for CLS there is an advantage in just copying things into the table: if the nodes that shift disappear from DOM the attribution disappears from the console.logs... but if you copy it to a table right as the entries are fired, those copies will stay. It actually helped me a second ago already.) In other words, +1 to land tables as they are an iterate afterwards. |
Extensions cannot access The same thing happens with the production version except you don't get a nice Error screen and instead have to use Inspect Views to see this: |
We could add these URLs to the manifest to avoid this error. But this is more a knock on from #104 rather than this PR. And it's more obvious as there is an Error screen for local development and you're looking at it a lot. |
I played around with I think we want this to be the super simple "easy" path for people wanting a bit more details than the HUD gives, but that don't want to hunt around in the Performance Observer entry that they currently have. |
I think for live tracing (e.g. for INP) where we show EVERY event doing it collapsed makes sense. But I was seeing that as a separate option for future. I was seeing these tables as more of a summary view for (ideally only one per metric). Because of the way web-vitals.js works we may see a few more (e.g. an initial LCP and then another LCP) but hopefully not so many that collapsing would be necessary. But agree people should switch this view off when looking at the future live trace view. |
Agreed with Michal to merge as is an iterate if needs be. |
Progress on #110
Fixes #112
Fixes #77
Progress on #86
This PR makes the following changes:
Here's what that looks like: