-
Notifications
You must be signed in to change notification settings - Fork 180
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): Summary page timeline #11393
base: main
Are you sure you want to change the base?
feat(nimbus): Summary page timeline #11393
Conversation
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.
Nice! On the current summary page the Timeline section shows a current state label (which I think is what this is replacing), but also a timeline with Start | Enrollment End | End dates. Will that timeline come later? How will it fit in with this? Or since it looks like there are some design docs, I could look there if they answer my questions and you don't mind sharing a link!
Also left you a question on the date logic and a suggestion for the state titles.
preview_changelog = ( | ||
self.changes.filter(new_status=self.Status.PREVIEW) | ||
.order_by("changed_on") | ||
.first() |
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.
Should this be first or last? If it changes out of preview and back into preview would we want to show the first or most recent date?
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.
(and same question for draft_date
)
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.
Ya a good question, I can think of reasons to want to do either. I'd slot this under 'pick one and wait for user feedback'.
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.
yes I had the same doubt and thought to use the first one, but we can change depending who users want to see it
<div class="d-flex align-items-center justify-content-between position-relative"> | ||
<!-- Draft Status --> | ||
<div class="border border-dark rounded p-3 text-center flex-grow-1 mx-2 {% if experiment.status == 'Draft' %}bg-primary text-white{% else %}bg-light{% endif %}"> | ||
<div>Draft</div> |
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'd suggest making these something like <h5>
so they stand out from the dates a bit.
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.
sure sure
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.
@yashikakhurana Yay!!!! Let's iterate on this together over zoom. |
@jaredlockhart @mikewilli new ui |
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.
Coming along! Couple problems:
- Name is bleeding under the timeline
- The dates are unreadable
- The background of some status blocks is grey and others are white
Maybe instead of having only the current status filled in and the rest be white, we should do it more like, each gets filled in, the current one is brightest, and the rest are grey like this?
also i tried moving the dates out from the statuses just to make it less visually clunky, i dunno, worth playing with.
Try generating experiments from each lifecycle to see what it looks like in every state.
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.
Yes looks fantastic!!! But I just realized we can simplify this.
<div class="col-6"> | ||
<ul class="list-group list-group-horizontal justify-content-between mb-3"> | ||
<!-- Draft Status --> | ||
<li class="list-group-item flex-fill text-center d-flex flex-column justify-content-center {% if experiment.status == 'Draft' and experiment.publish_status == 'Idle' %} bg-primary text-light {% else %} mode-sensitive text-dark {% endif %}"> |
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.
def timeline_dates(self):
return [
(self.Status.DRAFT.value, self.......),
.....
]
<ul class="....">
{% for status, date in experiment.timeline_dates %}
<li class="list-group-item .....>
<strong>{{ status}}</strong>
<small>{{ date|default:"---" }}</small>
</li>
{% endfor %}
</ul>
Oh and if you get a chance I also realized, once we hit LIVE we should put the 'computed_end_date' under |
From my comment:
With the latest updates, I think Start date is covered by Live, and End is covered by Complete, but will we show the Enrollment End date on this page or anywhere else? By the way, I love all the changes, the page is looking great! |
good question, let me see how we can incorporate that in this design, I will file a separate ticket to add Total duration, Enrollment days and enrollment end date |
Because
This commit
Note: On the same timeline, in the next PR, I will add the functionality to go back from preview to draft or preview to launch
Fixes #11361
Future design