-
Notifications
You must be signed in to change notification settings - Fork 9
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(nps): update UI #1370
feat(nps): update UI #1370
Conversation
!run-chromatic |
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.
More or less ok with the code, but here is my assumption of the flow here:
Currently, the REACT_APP_SHOW_NPS_FORM
is set to true, and the NPS forms are shown once every 3 (or 1 or 4) week. When we set the env var to be false, the timer in their local storage does not reset. Ie, this would mean that when ops/product wants to get feedback, setting the value to true
does not guarantee that we will be showing NPS to our users. (at the worst case the turnaround time is 4 weeks). as long as ops is aware of this flow this is fine (and tbh this flow is intuitive since we dont want to spam users with the feedback)
If this assumption is not correct, please dismiss my review
other nits: if prs are to be reviewed with relevant BE prs, please link them ya! my assumption here is that the relevant be pr is this, let me know if this is wrong!
} from "@chakra-ui/react" | ||
import { PropsWithChildren } from "react" | ||
|
||
const RATING_SCALE = 11 |
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.
damn TIL NPS starts from 0
const [lastSeen, setLastSeen] = useFeedbackStorage() | ||
// NOTE: Either this is the first time the user has ever seen the survey | ||
// or that the user has seen the survey but it has been more than 3 weeks. | ||
// Because we toggle the survey on every month, this indicates that they should |
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.
hmm shall we be precise on the frequency haha
7 days vs 3 weeks vs 1 mth
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.
2 things here - frequency of survey is 1 month, duration is 1 wek
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.
@kishore03109 add a bit of explanation of my understanding here:
- duration of survey is 1 week defined by the constant
NPS_SURVEY_DURATION
- the frequency of survey is controlled by manually turning on and off of the env var
REACT_APP_SHOW_NPS_FORM
@seaerchin correct me if I'm wrong.
6f6d921
to
37fb178
Compare
!run-chromatic |
bee00a9
to
cc0fcce
Compare
NOTE: There's an extra commit because the previous PR was merged into
develop
Please skip
useFeedbackDisclosure
To be reviewed w/ BE PR (here)
Problem
Previously we were using FormSG as a backend. This updates the nps ui to the one in the figma and hits a backend endpoint.
Solution
LoginContext
so we know theuserType
Tests
Notes
This PR is blocked by the design system upgrade