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.

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

Asset event attrs in dom #1433

Draft
wants to merge 102 commits into
base: master
Choose a base branch
from

Conversation

eoghanmurray
Copy link
Contributor

@eoghanmurray eoghanmurray commented Mar 25, 2024

This is for evaluation by Justin and is an extension of #1239

Copy link

changeset-bot bot commented Mar 25, 2024

🦋 Changeset detected

Latest commit: 1470f1a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb-snapshot Major
rrweb Major
rrdom Major
@rrweb/types Major
rrdom-nodejs Major
rrweb-player Major
@rrweb/web-extension Major
rrvideo Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@eoghanmurray eoghanmurray force-pushed the asset-event-attrs-in-dom branch 5 times, most recently from 5fdfd37 to dfcfee4 Compare April 3, 2024 10:34
Juice10
Juice10 previously requested changes Apr 4, 2024
Copy link
Contributor

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

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

Really loving this @eoghanmurray! I fixed your typing issues for you

packages/rrweb/src/record/observers/asset-manager.ts Outdated Show resolved Hide resolved
packages/rrweb/src/record/observers/asset-manager.ts Outdated Show resolved Hide resolved
packages/rrweb/src/replay/index.ts Outdated Show resolved Hide resolved
@eoghanmurray eoghanmurray force-pushed the asset-event-attrs-in-dom branch 2 times, most recently from e632690 to 5a23bf2 Compare April 10, 2024 15:14
Juice10 and others added 8 commits April 22, 2024 23:02
Co-authored-by: Eoghan Murray <eoghan@getthere.ie>
…the AssetManager needs to interrogate other attributes to determine how to manage, e.g. <link with href depending on rel="stylesheet"
…tributes instead of storing config on the Meta event

 - a better approach as discussed with Justin and Yanzhen
…n determine that Assets won't be arriving later
@eoghanmurray eoghanmurray force-pushed the asset-event-attrs-in-dom branch 3 times, most recently from 5b07eca to 674ca42 Compare April 23, 2024 10:16
@eoghanmurray eoghanmurray dismissed Juice10’s stale review April 23, 2024 10:17

He did the thing requested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants