-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: new filters for nps trends #28759
base: fix/nps-trend-chart-on-surveys
Are you sure you want to change the base?
feat: new filters for nps trends #28759
Conversation
Size Change: 0 B Total Size: 1.21 MB ℹ️ View Unchanged
|
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.
PR Summary
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
date_from: dayjs(values.survey.created_at).format('YYYY-MM-DD'), | ||
date_to: values.survey.end_date | ||
? dayjs(values.survey.end_date).format('YYYY-MM-DD') | ||
: dayjs().add(1, 'day').format('YYYY-MM-DD'), |
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 think we cna use a const here for YYYY-MM-DD
and reuse within the whole page
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.
left a few comments otherwise LGTM, much better!
Problem
NPS Trends lacks personality, and sometimes it's just an huge waste of space
Changes
see before vs after
adds specific filters options for NPS charts:
Does this work well for both Cloud and self-hosted?
N/A
Tests
Smoke tests locally
Unit tests for new logic
Checked if all tests are still running