Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Save a reference to the LCP and CLS elements in case they are removed from the DOM #562

Open
wants to merge 14 commits into
base: v5
Choose a base branch
from
Open

Save a reference to the LCP and CLS elements in case they are removed from the DOM #562

wants to merge 14 commits into from

Conversation

tunetheweb
Copy link
Member

Fixes #561

@tunetheweb tunetheweb mentioned this pull request Nov 6, 2024
Open
@tunetheweb
Copy link
Member Author

Test here: https://disappearing-cwv-node.glitch.me/index.html

Maybe we should add this to the test suite?
And maybe we should do this for CLS too?

philipwalton
philipwalton requested changes Nov 6, 2024
View reviewed changes
Copy link
Member

@philipwalton philipwalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add this to the test suite?

Yes, I think we want a test for this behavior. It should be possible to test.

src/attribution/onLCP.ts Outdated Show resolved Hide resolved
rviscomi
rviscomi reviewed Nov 6, 2024
View reviewed changes
src/attribution/onLCP.ts Outdated Show resolved Hide resolved
@tunetheweb tunetheweb changed the title Save a reference to the LCP element in case it is removed from the DOM Save a reference to the LCP and CLS elements in case they are removed from the DOM Nov 6, 2024
@tunetheweb tunetheweb mentioned this pull request Nov 6, 2024
Open
@tunetheweb
Copy link
Member Author

Yes, I think we want a test for this behavior. It should be possible to test.

Added tests for LCP, CLS and even the INP scenario added in #477

@dmitiiv
Copy link

dmitiiv commented Nov 7, 2024

Can you add the same logic for metric -> lcpEntry -> element?

@tunetheweb
Copy link
Member Author

tunetheweb commented Nov 7, 2024 •
edited
Loading

I've added it as a new attribution.elementNode entry as we don't want to amend the actual entry.

nicjansma
nicjansma reviewed Nov 18, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
philipwalton
philipwalton reviewed Dec 10, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
src/attribution/onCLS.ts Outdated Show resolved Hide resolved
src/attribution/onLCP.ts Outdated Show resolved Hide resolved
src/types/lcp.ts Outdated Show resolved Hide resolved
src/types/lcp.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rviscomi rviscomi rviscomi left review comments

@nicjansma nicjansma nicjansma left review comments

@philipwalton philipwalton philipwalton requested changes

Assignees

@philipwalton philipwalton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tunetheweb @dmitiiv @philipwalton @nicjansma @rviscomi