-
Notifications
You must be signed in to change notification settings - Fork 12
feat: handle guides live preview content #730
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
feat: handle guides live preview content #730
Conversation
🦋 Changeset detectedLatest commit: d397b4d The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ac3d799 to
e6effa0
Compare
c8dd940 to
f854611
Compare
e6effa0 to
5e8972a
Compare
f854611 to
626a881
Compare
thomaswhyyou
left a comment
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.
A few comments/questions.
| const debugStateChanged = | ||
| JSON.stringify(currentDebugParams) !== JSON.stringify(newDebugParams); |
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.
Hm I feel like JSON.stringify() can be brittle, for example:
JSON.stringify({a:1, b:2}) === JSON.stringify({b:2, a:1}) // false
For this I wonder if we can just check the key vals individually with a helper like checkDebugStageChanged(), or use something like fast-deep-equal we use in react-core? Given only two params, maybe just do this manually for now.
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 point, I'll just do a manual check especially since we actually only care whether or not guide key is present or not (since either way, we're just sending 'force all guides' to SB)
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.
since we actually only care whether or not guide key is present or not (since either way, we're just sending 'force all guides' to SB)
Double checking, don't you send preview_session_id for subscribe() also in Line 366 though?
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.
I meant that specifically when comparing the guide key we just care about presence, but yeah, we still want to compare the rest of the debug state, here's what I added:
private checkDebugStateChanged(a: DebugState, b: DebugState): boolean {
return (
Boolean(a.forcedGuideKey) !== Boolean(b.forcedGuideKey) ||
a.previewSessionId !== b.previewSessionId
);
}
721de22 to
f0cf0d4
Compare
f0cf0d4 to
e048ef0
Compare
df729b7 to
d397b4d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
==========================================
+ Coverage 63.92% 63.99% +0.06%
==========================================
Files 185 185
Lines 7513 7549 +36
Branches 900 907 +7
==========================================
+ Hits 4803 4831 +28
- Misses 2684 2692 +8
Partials 26 26
|

Description
Handles live preview update events and uses preview content if available for the given guide.
Checklist