Closed vitebo closed 3 months ago
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
There are some linter issues.
You can find/fix them by running yarn prettier --write .
I now realized that the PR's commit history has become huge, if needed I can clean up this timeline before the merge, just let me know
I now realized that the PR's commit history has become huge, if needed I can clean up this timeline before the merge, just let me know
No need to clean up. It's just part of the PR and a good documentation about the thoughts behind and the progress as well
@vitebo did you already sign the Contributor License Agreement?
@vitebo did you already https://github.com/grafana/faro-web-sdk/pull/595#issuecomment-2118994584?
oops, that was missing. I signed now
I can even see now on drone.grafana.net
(apparently I couldn't access it before) that some lint rules are breaking, I'll fix it here
@codecapitano I did the sync here because apparently it was the only check
that was missing, but you will probably need to release the CI again https://github.com/grafana/faro-web-sdk/pull/595/commits/626c6a73b40f82f65d05c4f0b6fce19e26dfe105
Great! I've made the change here: https://github.com/grafana/faro-web-sdk/pull/595/commits/ef356bc9710255117c5e832c2592b3be7cd3eaaf
One thing that bothers me is that the option is called trackWebVitalAttribution
instead of trackWebVitalsAttribution
, as suggested here: https://github.com/grafana/faro-web-sdk/pull/595#discussion_r1615898424
This creates an inconsistency in the codebase, with it being referred to as webVital
in some places and webVitals
in others. Looking at the Google Chrome repo, they always use webVitals
.
@codecapitano do you see any problem with standardizing it here as well? I can make this change today if it makes sense
Great! I've made the change here: ef356bc
One thing that bothers me is that the option is called
trackWebVitalAttribution
instead oftrackWebVitalsAttribution
, as suggested here: #595 (comment)This creates an inconsistency in the codebase, with it being referred to as
webVital
in some places andwebVitals
in others. Looking at the Google Chrome repo, they always usewebVitals
.@codecapitano do you see any problem with standardizing it here as well? I can make this change today if it makes sense
Hey @vitebo yes please rename it! When I wrote the suggestion for the name it was a typo that the "s" was missing. Great catch!
Approved @vitebo thank you so much and congrats to your first Faro contribution. 🥳 Great work!
Why
This is a feature that will be useful as discussed in this issue https://github.com/grafana/faro-web-sdk/issues/593
What
captureWebVitalsAttribution
option in thegetWebInstrumentations
function to decide which vitals instrumentation to loadLinks
https://github.com/grafana/faro-web-sdk/issues/593
Checklist