-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Buffer modifications to virtual stylesheets #618
Conversation
Restore skip duration Use virtualStyleRulesMap to re-populate stylesheet on Flush event Clear virtualStyleRulesMap after flush applied
…mer was immediately started and reached the snapshot before the setTimeout returned
…se we'll ignore it after scrubbing (restarting play head at a particular time). This is a problem if mutations have altered the player state, and we try to replay those mutations, so we e.g. try to remove an element that has already been removed because we haven't reset the FullSnapshot state
…s-stylesheet-insert-rule' into css-rules
…ot-double-play' into css-rules
Only applies changes on flush. `virtualStyleRulesMap` now works with strings instead of CSSRules. CSSRules can only be via made `.insertRule` on CSSStyleSheet in most browsers. And `new CSSStyleSheet()` only works in Chrome currently.
Although it will be a little easier if we can have two PRs for the stylesheet buffer and the new testing infra, I finally finish the review:) @Juice10 The testing part looks good to me! And huge thanks for picking @VladimirMilenko's work. Nice teamwork. |
a.attributes.style = a.attributes.style!.replace( | ||
coordinatesReg, | ||
'$1: Npx', | ||
); |
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.
@Juice10 I appreciate the wish to apply standardised formatting to things, and I'm going to be doing that now with my own commits whenever I can (now that you've shown me how with prettify!)
however these type of non-functional changes do have a cost in terms of conflict resolution, e.g. when there's a parallel change in an open pull request (or in a custom commit which is to be maintained on top of the master)
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.
You are correct, we should include it in a CI step so it gets applied to all
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.
Do you mean like a failing test, so that when you create a pull request you know to make the formatting changes yourself?
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.
@eoghanmurray Correct!
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.
There might be certain hooks we could use to do this automatically as well, but I'm not sure what would be a good one for that.
Fixes #563 #525 and (probably) fixes #614
Builds on #529. Special thanks to @VladimirMilenko for doing the initial work on this.
Moves all mutations to virtual stylesheets that aren't part of the dom to
virtualStyleRulesMap
.When stylesheets get moved to the virtual dom their styles are also backed up to the
virtualStyleRulesMap
.This PR also adds replayer snapshot testing, and some replay unit tests for restoring virtual styles.