Conversation
|
|
There was some recent confusion on what the Overview section of the metric details modal is showing -- can/should we tack something on to this PR to update that too? Or file a new ticket? |
|
i think we can just include that change in this pr and broaden the scope/title to covering general improvements to analysis window representation |
82bd90e to
4ebfe61
Compare
|
We discussed how 3DR and 2-week retention should be labeled in the UI. While there was some discussion around whether different time windows (e.g. Overall vs Day 4 for 3DR, Overall vs Week 2 for retention) should be shown, the consensus was to keep this ticket scoped to its original goal: adding a label that reflects the currently displayed window. Any broader changes to retention window semantics can be handled in a follow-up ticket. For the current implementation, we've decided labels such as "Day 4" for 3DR and "Week 2" for retention accurately describe the existing behavior. |
mikewilli
left a comment
There was a problem hiding this comment.
Super simple, love it! There are a couple potential scenarios I'd like to avoid that I left comments about. Please feel free to let me know if the concerns are unfounded, otherwise we should ensure we guard against them before merging to avoid potentially displaying incorrect info.
| if ( | ||
| kpi["slug"] == NimbusConstants.RETENTION_3_DAYS | ||
| or kpi["slug"] == NimbusConstants.RETENTION_3_DAYS_DESKTOP | ||
| ): | ||
| kpi["displayed_window"] = "Day 4" |
There was a problem hiding this comment.
Isn't this unnecessary too?
There was a problem hiding this comment.
sorry what did you mean by unnecessary? isn't it still true that 3dr only shows the day 4 window index? if that isn't there the displayed window for it will be either daily, weekly, or overall all of which presumably wouldn't be correct
There was a problem hiding this comment.
Oh right, I keep forgetting that the label by default is not specific to each metric based on the data that comes in. Ok, let's go with this then.
Because
This commit
Fixes #14850