-
Notifications
You must be signed in to change notification settings - Fork 186
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(nimbus): Enrollment and complete days #11711
feat(nimbus): Enrollment and complete days #11711
Conversation
…rana/experimenter into 11621/enrollment_data
Blocked on #11709 |
experimenter/experimenter/nimbus_ui_new/templates/nimbus_experiments/timeline.html
Outdated
Show resolved
Hide resolved
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.
Oh, I think we should only ever have one of the status boxes highlighted at a time?
Also we could change 'live' to 'enrolling' and 'enrollment end' to 'observing' to make the titles shorter and maybe a little clearer?
Also I like adding teh 'X days' to them, we should put that on all of them, but we can do that seprately.
made the changes to show only one box active at a time and changed the name To add days in each section- I filled the ticket for that #11747 |
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 with the change from "Live" to "Enrolling" and "Enrollment End" to "Observing" it's muddled the "# days" indicators. To me it's a bit confusing that "Observing" shows the # of days that were spent enrolling, and this should instead go under "Enrolling", with a separate computed duration for the observation (end_date - enrollment_end_date
).
@mikewilli I changed and moved the days around, do you want to take another look? Also thank you for bringing that up |
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.
LGTM thanks!
Because
This commit
Fixes #1
1710