Skip to content

Conversation

@0jinxing
Copy link
Contributor

support selection change record and replay

@Yuyz0112
Copy link
Member

@0jinxing Thanks! This looks interesting, could you provide more information about the use case of this feature, and a short video demo for the replay effect?

BTW, e2e test cases are required.

@YunFeng0817
Copy link
Member

@0jinxing Thanks for your excellent feature contribution.
I made a simple demo with your code and the effect is fantastic!

record.mp4
replay.mp4

I am looking forward to seeing the complementation and merge of this PR!

@0jinxing 0jinxing force-pushed the master branch 2 times, most recently from dc5d8d6 to c3295f0 Compare August 3, 2022 09:35
@0jinxing 0jinxing requested a review from YunFeng0817 August 3, 2022 09:52
@0jinxing 0jinxing requested a review from YunFeng0817 August 3, 2022 11:38
Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

@0jinxing Thanks for your quick modification and reply! Hope to see it merged soon.

@YunFeng0817 YunFeng0817 requested a review from Yuyz0112 August 3, 2022 13:49
@Yuyz0112
Copy link
Member

Yuyz0112 commented Aug 6, 2022

Thanks, let's merge it!

@Yuyz0112 Yuyz0112 merged commit b2d5689 into rrweb-io:master Aug 6, 2022
@Juice10 Juice10 added the 2.0 label Dec 1, 2022

updateSelection();

return on('selectionchange', updateSelection);
Copy link
Contributor

Choose a reason for hiding this comment

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

The on function here expects a third 'target' argument; I presume it should be doc?

Copy link
Contributor

Choose a reason for hiding this comment

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

@0jinxing can you confirm that it should be doc passed as the third argument?
Otherwise it's trying to addEventListener on undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

@YunFeng0817 I'm not sure how this has escaped everyone's notice if it's been merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants