-
Notifications
You must be signed in to change notification settings - Fork 142
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
🐛 Fix replay visual viewport resize support #2891
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2891 +/- ##
==========================================
+ Coverage 93.75% 93.77% +0.01%
==========================================
Files 266 267 +1
Lines 7575 7577 +2
Branches 1685 1685
==========================================
+ Hits 7102 7105 +3
+ Misses 473 472 -1 ☔ View full report in Codecov by Sentry. |
55176b3
to
9fb112d
Compare
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
expect(getEmittedRecords()[0].type).toBe(RecordType.Focus) | ||
}) | ||
|
||
it('adds a Focus record on when taking a full snapshot', () => { |
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.
Now covered here:
it('full snapshot records should contain Meta, Focus, FullSnapshot', () => { |
expect(getEmittedRecords()[1].type).toBe(RecordType.Focus) | ||
}) | ||
|
||
it('set has_focus to true if the document has the focus', () => { |
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.
Already covered here:
it('collects focus', () => { |
expect((getEmittedRecords()[1] as FocusRecord).data.has_focus).toBe(true) | ||
}) | ||
|
||
it("set has_focus to false if the document doesn't have the focus", () => { |
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.
Already covered here:
it('collects blur', () => { |
it('adds an initial Focus record when starting to record', () => { | ||
startRecording() | ||
expect(getEmittedRecords()[1]).toEqual({ | ||
type: RecordType.Focus, | ||
timestamp: jasmine.any(Number), | ||
data: { | ||
has_focus: true, | ||
}, | ||
}) | ||
}) | ||
|
||
it('adds a Focus record on focus', () => { | ||
startRecording() | ||
emitSpy.calls.reset() | ||
|
||
window.dispatchEvent(createNewEvent('focus')) | ||
expect(getEmittedRecords()[0].type).toBe(RecordType.Focus) | ||
}) | ||
|
||
it('adds a Focus record on blur', () => { | ||
startRecording() | ||
emitSpy.calls.reset() | ||
|
||
window.dispatchEvent(createNewEvent('blur')) | ||
expect(getEmittedRecords()[0].type).toBe(RecordType.Focus) | ||
}) |
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.
Basically covered here:
it('full snapshot records should contain Meta, Focus, FullSnapshot', () => { |
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.
Good catch
@@ -422,6 +369,95 @@ describe('record', () => { | |||
}) | |||
}) | |||
|
|||
describe('should collect records', () => { |
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.
👏 praise: looks great
Motivation
A regression has been introduced in the PR: the replay visual viewport resize support has been removed.
Changes
Testing
I have gone over the contributing documentation.