Conversation
charisk
left a comment
charisk
left a comment
There was a problem hiding this comment.
Just a partial review from me for now - I'll come back to it tomorrow.
| // assume anything less than 0 is a zero | ||
| if (!millis || millis < ONE_MINUTE_IN_MS) { | ||
| return 'Less than a minute'; | ||
| if (!millis || millis < ONE_SECOND_IN_MS) { |
There was a problem hiding this comment.
This is changing existing behaviour and I'm not sure we want that. Have you discussed with Product/Design at all?
There was a problem hiding this comment.
No, I haven't. I figured this would be fine since it's only used in 1 other place right now: the panel for a remote query. We are essentially recreating that panel with a better design and support for live results, so I don't think it matters that the duration for queries to change from less than 1 minute to 12 seconds. This will also only happen for failed or cancelled queries since other queries will almost always take more than 1 minute to complete.
There was a problem hiding this comment.
I think we should aim to flag anything that changes existing behaviour with Product/UX. I expect it'll be okay (and I agree with you that the change makes sense), but I want to make sure we don't surprise anyone :)
| // assume anything less than 0 is a zero | ||
| if (!millis || millis < ONE_MINUTE_IN_MS) { | ||
| return 'Less than a minute'; | ||
| if (!millis || millis < ONE_SECOND_IN_MS) { |
There was a problem hiding this comment.
I think we should aim to flag anything that changes existing behaviour with Product/UX. I expect it'll be okay (and I agree with you that the change makes sense), but I want to make sure we don't surprise anyone :)
charisk
left a comment
charisk
left a comment
There was a problem hiding this comment.
Thank you for all the hard work getting this over the line!
2 participants
This adds the variant analysis stats component. It adds a story for each possible state so it should be easy to check against the design.
The
VariantAnalysisHeadercomponent has a lot of props now; we might want to refactor it. However, I think that will be easier once we actually have an interface from which we can get the data, so I've left it like this for now.Checklist
ready-for-doc-reviewlabel there.