-
Notifications
You must be signed in to change notification settings - Fork 216
feat(product tours): enable event and action triggers #2792
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(product tours): enable event and action triggers #2792
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
this is basically copied from survey-event-receiver
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.
but ... this is a little risky, so if you'd rather keep surveys as-is and only use this for product tours (for now) -- happy to do that as well
i tested it manually, but our automated testing is a bit lacking here
edit: our tests are actually better than i thought lol. i think this is ok... here is also a diff of the original survey rx file with the new abstract one https://www.diffchecker.com/ZdVJNFsc/
710a254 to
81d745b
Compare
|
Size Change: +16.1 kB (+0.3%) Total Size: 5.32 MB
ℹ️ View Unchanged
|
81d745b to
7ee3431
Compare
98d9da6 to
6e6f6ab
Compare
6e6f6ab to
ea75a4c
Compare
ea75a4c to
988f0bc
Compare
988f0bc to
bc2ff02
Compare
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.
this is before my time, but pretty sure this was flakey because we didn't wait for the api response... i had the same issue with some other tests i added recently. i'd like to keep these enabled
| queueTourWithDelay(tourId: string, delaySeconds: number, reason?: ProductTourRenderReason): void { | ||
| logger.info(`Queueing tour ${tourId} with ${delaySeconds}s delay`) | ||
|
|
||
| const timeoutId = setTimeout(() => { | ||
| this._pendingTourTimeouts.delete(tourId) | ||
| logger.info(`Delay elapsed for tour ${tourId}, showing now`) | ||
| this.showTourById(tourId, reason) | ||
| }, delaySeconds * 1000) | ||
|
|
||
| this._pendingTourTimeouts.set(tourId, timeoutId) | ||
| } |
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.
nit: might be worth adding this.cancelPendingTour(tourId) at the start of queueTourWithDelay as a defensive measure. i know _showOrQueueTour already checks isTourPending before calling this, but since queueTourWithDelay is public, someone could call it directly and accidentally overwrite a timeout without cancelling
the previous one
|
|
||
| const logger = createLogger('[Product Tour Event Receiver]') | ||
|
|
||
| export class ProductTourEventReceiver extends EventReceiver<ProductTour> { |
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.
would be good to have some unit tests for ProductTourEventReceiver specifically - especially around _isItemPermanentlyIneligible since it behaves differently than the survey version (checking localStorage for completed/dismissed state)

Problem
product tours should be trigger-able through events or actions
Changes
make them!
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file