feat: date filter experiment #10462

kappa90 · 2022-06-23T16:41:17Z

Problem

Related: https://github.com/PostHog/product-internal/pull/305

Experiment could close: #9727, #9453, #9027, #8059, #7625

Does not fix (up for discussion if still relevant): #9664

Following the conversion in issue #9727, and the other mentioned issues, the entire date picker flow has been improved.

All changes are behind a feature flag, in order to be able to test them as an experiment.

Changes

Behind feature flag:

Default rolling time range is 30 days (from 7 days)
A new rolling range picker allows the user to select a rolling date range in the format: {number} {period}.
- Rolling date ranges periods are: days, weeks, months, quarter
Default option Last 3 days is removed as it becomes the default for the rolling date range picker
All options have tooltips on hover showing the formatted selected range
By selecting an option, the Grouped by interval is automatically selected. In particular:
- anything equal or below 3 days, is grouped by hour
- equal or below 3 months, is grouped by day
- above, is grouped by month
Both fixed rolling date ranges and fixed date ranges show the formatted range in the button label
When selecting custom fixed ranges, the calendar picker is pre-populated with today's date as end date. The number of clicks to select a fixed range is thus reduced from 7 to 4.

Not behind a feature flag (fix):

Insights API now always exposes a date_from filter, to indicate what is the date window, which, if not selected from the UI, is initially set from backend. Not exposing this parameter can lead to buggy states in the UI, which doesn't know information about the insight date range.

How did you test this code?

Cypress tests for the new rolling range picker, and the automatic interval selection.

GIF/Images

New flow:

neilkakkar · 2022-06-23T17:08:05Z

Minor flyby feedback: This PR popped up in my email with the fix(experiment) title that gave me a mini-scare because I rushed to check what went wrong with the experiments product.

With conventional commits, the things in the bracket is the scope, a.k.a what part of the project you're touching, so fix(date-filters) would be a lot more ideal title. :)

This also sounds like a full blown feature, hence feat... except the part not behind the ff which is a fix, which also might be a signal that this PR is too huge (but I haven't looked at how big the fix actually is, so idk, 🤌 🧂 )

kappa90 · 2022-06-23T17:12:30Z

Minor flyby feedback: This PR popped up in my email with the fix(experiment) title that gave me a mini-scare because I rushed to check what went wrong with the experiments product.

With conventional commits, the things in the bracket is the scope, a.k.a what part of the project you're touching, so fix(date-filters) would be a lot more ideal title. :)

This also sounds like a full blown feature, hence feat... except the part not behind the ff which is a fix, which also might be a signal that this PR is too huge (but I haven't looked at how big the fix actually is, so idk, pinched_fingers salt )

Ah, sorry, let me remove that, I wasn't sure how to flag it as an experiment. It's a fix, but more of a UX fix, not a bug fix.

The PR seems huge because there's code duplication, because of the feature flag, but the actual changes are not really that big.

liyiy · 2022-06-23T19:56:51Z

I think your linter ran through everything which is why there's so many files changed 😮 , you can run black on just a single file at a time

kappa90 · 2022-06-23T21:45:48Z

I think your linter ran through everything which is why there's so many files changed open_mouth , you can run black on just a single file at a time

ah, my bad, I removed anything that wasn't part of the original commits

benjackwhite · 2022-06-24T07:35:21Z

Checked out locally and I get some weird layout going on @kappa90 are you getting this locally as well? Maybe something weird on a merge from master?

benjackwhite · 2022-06-24T07:40:11Z

@kappa90 I think this might be the culprit - https://github.com/PostHog/posthog/pull/10462/files#diff-c908934c4df2c03e9cb3f37eef8e2b4c921b0a0bd82c1c088b04c2d595a5f4bfR1

Should be .rolling-date-range-filter I think

kappa90 · 2022-06-24T07:45:01Z

@kappa90 I think this might be the culprit - https://github.com/PostHog/posthog/pull/10462/files#diff-c908934c4df2c03e9cb3f37eef8e2b4c921b0a0bd82c1c088b04c2d595a5f4bfR1

Should be .rolling-date-range-filter I think

Ah shoot, fixing it, I refactored some files and class names yesterday to include "rolling" and that css class got out. Thanks!

posthog/utils.py

frontend/src/scenes/saved-insights/SavedInsights.tsx

benjackwhite · 2022-06-24T07:54:48Z

frontend/src/lib/components/LemonInput/LemonInput.tsx

@@ -15,8 +15,8 @@ export interface LemonInputProps
    value?: string | number
    defaultValue?: string
    placeholder?: string
-    onChange?: (newValue: string) => void
-    onPressEnter?: (newValue: string) => void
+    onChange?: (newValue: string | number) => void


Wondering if this has any wider impacts. Running yarn typescript:check definitely throws some issues up.
Would be awesome if we can infer the type using a generic for value?

I reverted it, and it shows other problems, I think the LemonInput component needs to be refactored. I tried a couple things but couldn't figure it out.

Browser textfields only support strings. Even with type="number", you need to access a special variable input.valueAsNumber to get the numeric value, complicating everything. If we add another component for number inputs, it's best to encapsulate this in a LemonInputNumeric component.

Made this to solve the issue: #10533

benjackwhite · 2022-06-24T07:55:58Z

frontend/src/lib/components/DateFilter/RollingDateRangeFilter.scss

+    }
+}
+
+.date-options-selector-popup {


Not a massive fan of this but I know why it's here...

Do you have any idea how could I apply a class to that popup otherwise?

mariusandra

Quick flyby before I dig in deeper: I see a lot of different types of dropdowns on the page.

How closely is the new dropdown following the design in figma? I don't have a link nearby to check now, but I think we are converging on the new lemony look everywhere.

This would look out of place with the other chart dropdowns, but even then the hover color is different now. Something to check?

kappa90 · 2022-06-24T13:14:16Z

Quick flyby before I dig in deeper: I see a lot of different types of dropdowns on the page.

How closely is the new dropdown following the design in figma? I don't have a link nearby to check now, but I think we are converging on the new lemony look everywhere.

This would look out of place with the other chart dropdowns, but even then the hover color is different now. Something to check?

Good catch, I built on existing drop-downs and didn't notice the inconsistencies, will fix this!

mariusandra

Few more comments

mariusandra · 2022-06-24T14:37:00Z

frontend/src/lib/components/DateFilter/DateFilterExperimentLogic.ts

+        propsChanged: (props) => {
+            // when props change, automatically reset the Select key to reflect the change
+            const { dateFrom, dateTo, defaultValue, dateOptions, isDateFormatted } = props
+            if (dateFrom && dayjs(dateFrom).isValid() && dayjs(dateTo).isValid()) {
+                actions.setCurrentKey(`${dateFrom} - ${dateTo}`)
+            } else {
+                const currKey = dateFilterToText(dateFrom, dateTo, defaultValue, dateOptions, false)
+                actions.setCurrentKey(
+                    isDateFormatted && !(currKey in dateOptions)
+                        ? dateFilterToText(dateFrom, dateTo, defaultValue, dateOptions, true)
+                        : currKey
+                )
+            }
+        },


I suggest one additional layer of safety here: generate the currentKey, save it as a variable, and only set it if it's not the same as the current currentKey. It's possible to get into loops when responding to events of all types, and it's safer to code defensively, minimising changes. Even if not now, always code with the assumption that you never know what other code will live with your code in the future. E.g. someone could add a bunch of rapidly changing props that trigger this event and the actions, without needing to.

Done, thanks for pointing that out!

mariusandra · 2022-06-24T14:38:50Z

frontend/src/lib/components/DateFilter/DateFilterExperiment.tsx

+            data-attr="date-filter"
+            bordered={bordered}
+            id="daterange_selector"
+            value={currentKey}


I was confused when I saw the currentKey prop. Knowing it's the value of the field cleared everything up. So could we rename it value? 🤔

Renamed this to value

mariusandra · 2022-06-24T14:39:49Z

frontend/src/lib/components/DateFilter/DateFilterExperiment.tsx

+    useEffect(() => {
+        window.addEventListener('mousedown', onClickOutside)
+        return () => {
+            window.removeEventListener('mousedown', onClickOutside)
+        }
+    }, [isOpen])


There's a useOutsideClickHandler helper. Could this be used instead?

I also feel like this onClickOutside handling is also slightly out of scope of this component, and could just be handled by <Popup> instead?

Made it so that Popup can now handle additional refs passed as props, so that it can manage nested popups, and removed all the click handling logic that was outside of it!

mariusandra · 2022-06-24T14:51:17Z

My local clickhouse is broken, so it might be related and work under normal conditions, but I can't select anything from the weeks/days dropdown:

clarkus · 2022-06-24T18:04:05Z

This is a massive improvement to the date picking workflow, @kappa90. I know there are still some implementation details being worked out, but generally based on the demo gif you shared this looks really well done. There are two topics I'd like to point out (also pointed out by you and @mariusandra):

Does not fix (up for discussion if still relevant): #9664

It's hard to know user intent. That issue assumes the user is wanting to select another fixed range. It's equally feasible that the user wants to select a rolling range. Given that it's two clicks to get back to the calendar picker, I'm not sure this should block shipping this change. If we were to show the calendar by default, and the user wanted a rolling range, they'd still have to click back to get to the menu that allows them to select a relative range. It's the same amount of effort either way and starting from the same menu does make it obvious how to get back to the calendar. This one might need more discussion before settling on a fix.

Marius pointed out:

Quick flyby before I dig in deeper: I see a lot of different types of dropdowns on the page.

This is happening all over the product even with features that we just shipped. I've been trying to call out opportunities to make these more consistent as we get new lemon components. I agree that it's a good and valuable change to make. I do think it should be distinct from this issue only because I'd like to see this change ship sooner than later. I've been working on longer term consistency changes for the insight results container at https://www.figma.com/file/gQBj9YnNgD8YW4nBwCVLZf/PostHog-App?node-id=11224%3A50863. Maybe we can schedule that for a follow up project?

clarkus

👍

…periment

mariusandra

Almost :)

frontend/src/lib/components/DateFilter/RollingDateRangeFilter.tsx

frontend/src/lib/components/LemonSelect.tsx

frontend/src/scenes/saved-insights/SavedInsights.tsx

frontend/src/toolbar/button/ToolbarButton.scss

frontend/src/lib/components/DateFilter/DateFilter.tsx

frontend/src/lib/components/DateFilter/DateFilterExperiment.scss

frontend/src/lib/components/DateFilter/dateFilterLogic.ts

kappa90 · 2022-07-04T11:25:28Z

We rescoped this experiment to only change the interface. In the next experiment we will change the default date_from to 30 days, from 7 days.

mariusandra

Hit the button!

mariusandra · 2022-07-04T11:43:58Z

frontend/src/lib/components/DateFilter/DateFilter.tsx

@@ -195,7 +195,8 @@ function DateFilterExperiment({
    dateOptions = dateMappingExperiment,
    isDateFormatted = true,
 }: RawDateFilterProps): JSX.Element {
-    const logicProps = { dateFrom, dateTo, onChange, defaultValue, dateOptions, isDateFormatted }
+    const key = useRef(uuid()).current


Nice trick! If we'd migrate to React 18, we could use the new useId here.

Makes me question why we need Kea here but in any case it works 😅

Using kea logic with components is a blurry subject, especially in the case of small components that share no state with the outside world. This could have been in pure React, and I wouldn't have minded.

However I do think it's beneficial to keep a strong line between code operating at the application data level (in kea) vs code at the view level (stateless interactive components, react templates around application data). I also know that everything expands in scope, and I've seen too many times how data in that useState call eventually wants to interact with data from elsewhere in the app, requiring a rewrite of the component into logic land. Hence I suggest to just save future you the trouble and write a logic directly.

Since you're considering work that might never happen, it's hard to draw a line. What if we wrote everything with useState, and want to go fancy and show a heatmap of events on the calendar, like google flights does for the price? Rewrite to a logic? Parallel system of network calls to load the data in React hooks? 🤷

Frontend trends come and go, and if we remain consistent in our approach and technologies, we'll build a more maintainable app in the long run.

macobo · 2022-07-04T11:50:51Z

Should this have a highlight tag + some prewritten release notes?

kappa90 requested review from mariusandra, clarkus, benjackwhite and marcushyett-ph June 23, 2022 16:41

kappa90 changed the title ~~Fix(experiment): date filter experiment~~ fix(experiment): date filter experiment Jun 23, 2022

kappa90 changed the title ~~fix(experiment): date filter experiment~~ fix: date filter experiment Jun 23, 2022

kappa90 changed the title ~~fix: date filter experiment~~ feat: date filter experiment Jun 23, 2022

kappa90 force-pushed the fix/date-filter-experiment branch from 77f3568 to 9718822 Compare June 23, 2022 21:44

benjackwhite requested changes Jun 24, 2022

View reviewed changes

kappa90 requested a review from benjackwhite June 24, 2022 10:12

mariusandra reviewed Jun 24, 2022

View reviewed changes

mariusandra requested changes Jun 24, 2022

View reviewed changes

clarkus approved these changes Jun 24, 2022

View reviewed changes

remove black reformatting

109c513

kappa90 force-pushed the fix/date-filter-experiment branch from 7c7a718 to 109c513 Compare June 29, 2022 12:48

kappa90 added 3 commits June 29, 2022 13:57

nested popup click handling

9ecd104

black reformat

2dd1b84

fix UI regressions

69e1c5c

kappa90 force-pushed the fix/date-filter-experiment branch from 95b97fe to 69e1c5c Compare June 29, 2022 16:31

fix test

e7309a5

kappa90 added 3 commits July 1, 2022 13:45

refactr useOutsideClickHandler to handle tagNames

2e54202

render popups in shadow root

9b2c438

Merge remote-tracking branch 'origin/master' into feat/date-filter-ex…

1771fef

…periment

kappa90 requested a review from mariusandra July 1, 2022 14:39

mariusandra requested changes Jul 1, 2022

View reviewed changes

kappa90 and others added 7 commits July 4, 2022 10:19

fixes after review

6f9afc4

add key to logic

c407887

remove 30 days default

67f7878

fix small bug

0b0f21a

Added datefilterlogic test

cff61f0

Fix margin for old date filter

71dfa35

Fixed key

ac0aeca

benjackwhite approved these changes Jul 4, 2022

View reviewed changes

add logic test

bb510a9

benjackwhite requested a review from mariusandra July 4, 2022 11:11

Fixed tests to be different from new default

4267e3d

mariusandra approved these changes Jul 4, 2022

View reviewed changes

benjackwhite merged commit dfc04af into master Jul 4, 2022

benjackwhite deleted the fix/date-filter-experiment branch July 4, 2022 11:46

joao-venue mentioned this pull request Jul 4, 2022

Improvement: Group by minute #10633

Open

kappa90 mentioned this pull request Jul 8, 2022

chore: remove date filter experiment #10688

Merged

clarkus mentioned this pull request Jul 15, 2022

Component regression for dashboard date controls #10829

Closed

3 tasks

kappa90 mentioned this pull request Jul 18, 2022

New date filter option: "Since..." #10727

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: date filter experiment #10462

feat: date filter experiment #10462

kappa90 commented Jun 23, 2022 •

edited

Loading

neilkakkar commented Jun 23, 2022

kappa90 commented Jun 23, 2022

liyiy commented Jun 23, 2022

kappa90 commented Jun 23, 2022

benjackwhite commented Jun 24, 2022

benjackwhite commented Jun 24, 2022

kappa90 commented Jun 24, 2022

benjackwhite Jun 24, 2022

kappa90 Jun 24, 2022

mariusandra Jun 24, 2022

kappa90 Jun 28, 2022

benjackwhite Jun 24, 2022

kappa90 Jun 24, 2022

mariusandra left a comment

kappa90 commented Jun 24, 2022

mariusandra left a comment

mariusandra Jun 24, 2022

kappa90 Jun 29, 2022

mariusandra Jun 24, 2022

kappa90 Jun 29, 2022

mariusandra Jun 24, 2022

mariusandra Jun 24, 2022

kappa90 Jun 29, 2022

mariusandra commented Jun 24, 2022

clarkus commented Jun 24, 2022 •

edited

Loading

clarkus left a comment

mariusandra left a comment

kappa90 commented Jul 4, 2022

mariusandra left a comment

mariusandra Jul 4, 2022

benjackwhite Jul 4, 2022

mariusandra Jul 4, 2022

macobo commented Jul 4, 2022

feat: date filter experiment #10462

feat: date filter experiment #10462

Conversation

kappa90 commented Jun 23, 2022 • edited Loading

Problem

Changes

How did you test this code?

GIF/Images

neilkakkar commented Jun 23, 2022

kappa90 commented Jun 23, 2022

liyiy commented Jun 23, 2022

kappa90 commented Jun 23, 2022

benjackwhite commented Jun 24, 2022

benjackwhite commented Jun 24, 2022

kappa90 commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra left a comment

Choose a reason for hiding this comment

kappa90 commented Jun 24, 2022

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra commented Jun 24, 2022

clarkus commented Jun 24, 2022 • edited Loading

clarkus left a comment

Choose a reason for hiding this comment

mariusandra left a comment

Choose a reason for hiding this comment

kappa90 commented Jul 4, 2022

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macobo commented Jul 4, 2022

kappa90 commented Jun 23, 2022 •

edited

Loading

clarkus commented Jun 24, 2022 •

edited

Loading