fix 14991: support custom comparison period start date #22267
Conversation
There was a problem hiding this comment.
Hi @mariusandra! Could you please review the UI and the comments I left in the PR?
| @@ -2003,6 +2007,7 @@ export interface TrendsFilterType extends FilterType { | |||
| // 7 days to remove weekly variation. Smoothing is performed as a moving average. | |||
| smoothing_intervals?: number | |||
| compare?: boolean | |||
| comparison?: Comparison | |||
There was a problem hiding this comment.
Once this change is rolled out I plan to remove the compare field and use comparison instead. If comparison is present, we should compare based on its internal fields.
For now these fields will work together for backward compatibility.
| interval = 'hour' | ||
| } | ||
| let diffBetweenDates = dateTo.diff(dateFrom, interval, /* float= */ true) | ||
| // This assumes that dateTo is now, so we need to look twice further to see |
There was a problem hiding this comment.
This assumption is not always true. I'll fix it in the next PRs to not overcomplicate this one
| /> | ||
| <RollingDateRangeFilter | ||
| pageKey={key} | ||
| dateFrom={comparisonRelativePeriodStart} |
There was a problem hiding this comment.
There's a problem here. When I update the insight range, the comparison period is not updated, because RollingDateRangeFilter stores the current values in the state. Here's a video of what I mean, "2 days" comparison period remains the same.
IIUC if I want to update the comparison period when the range updates, I need to
- create compareFilterLogic
- Connect it to the insightLogic; listen to the updateInsightFilter
- On updateInsightFilter call a method that would update RollingDateRangeFilter state from the new comparison period
Is that correct? If so, I suggest I implement it in the follow up PR, WDYT? Maybe there's a simpler way to do this?
|
|
||
| const DEFAULT_RELATIVE_START_DATE = '1d' | ||
|
|
||
| export const getDefaultComparisonPeriodRelativeStartDate = ( |
There was a problem hiding this comment.
This method is based on getComparePeriodDates. It's simpler and seems more consistent between "last 7 days" and "last 6 days"
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
|
@PostHog/team-product-analytics can someone have a look at this? I understand there's an alternative in the works? |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
Hi @nikitaevg, we shipped this last week: #22397 which covers these changes. I didn't see this PR until I had already built out the functionality. Closing this. |
Problem
Fix #14991
Changes
This is a draft of the frontend implementation.
Here's how it looks like.
Does this work well for both Cloud and self-hosted?
How did you test this code?