-
Notifications
You must be signed in to change notification settings - Fork 433
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: show Beamer NPS on tx success screen #2461
Conversation
Branch preview⏳ Deploying a preview site... |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
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’re on 🔥!
src/hooks/Beamer/useBeamerNps.ts
Outdated
const lastShown = window.Beamer.getCookie(LS_KEY) | ||
|
||
if (!lastShown) { | ||
window.Beamer.forceShowNPS() |
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.
Can you link to Beamer docs about this fn? Why do we "force" it?
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.
Have spoken with Beamer regarding this, as well as asked some other questions. I'll make this a draft for now and include what I hear back.
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.
As per our discussion, I've added comments to address this.
src/hooks/Beamer/useBeamerNps.ts
Outdated
// randomly showing on pages that we don't want it to | ||
// Note: this is not documented but confirmed by Beamer support | ||
if (!lastShown) { | ||
window.Beamer.forceShowNPS() |
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 could also unsubscribe right after.
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.
Code looks good.
Does it remember that a banner was shown or that it's been answered?
It checks whether it was shown, matching that of their API. I just refactored the code a little bit according to your suggestion above. |
The feature is fine. The popUp shows up for the first time when you propose a transaction and then it doesn't show up anymore. Note: A large number of users use a 1/x safes, so is likely that they only create and execute tx at the same time, so all those people will not see this beamer popup. |
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.
We shouldn’t show it for executions and signatures. Only for creations. Otherwise people will be rating completely different flows.
Recheck this after the last commit. Made sure it still works only with the Beamer checkbox enabled in preferences LGTM |
What it solves
Resolves #2432
How this PR fixes it
Beamer's NPS is now shown on the transaction
PROPOSED
.How to test it
Create a transaction, observing the NPS opening when the proposal succeeds.
Screenshots
Note: this is an old recording but the functionality should be the same, but on proposal.
Checklist