chore: date filter UI fixes #10673

kappa90 · 2022-07-07T17:14:26Z

Problem

Resolves #10640

Changes

Aligned the design of the Insight Config panel, now all buttons and popups use the Lemon library and have the same style
- For the Chart Filter, the button label still has the icon, which could be removed (see screenshots)
Removed wrong padding in the new Date Filter component
- ...and made the background color and selected state of the Rolling Date Picker coherent with the other buttons
Changed all date formatting and made it so that it cleverly drops the year on the starting date in case it's the same year as the end date
Added tooltips to the Calendar Picker

Changes that couldn't be implemented:

Avoid showing future months / years in the Calendar Picker
Show the current month on the right side of the Calendar Picker when opened (currently opens it to the left)

Why: the Calendar Picker is an antd component with limited customization, we would have to rewrite entirely the picker to enable these (out of scope for this PR)

How did you test this code?

Locally, and cypress

clarkus

Thanks for working on this. Here's what I'm seeing:

The date range select component doesn't show the active border color when focused. The border should show active blue when the field is focused. It looks like this might be affecting other selects in this context in your screenshots.
It looks like item 4 from Date picker workflow component improvements and alignments #10640 isn't included. The main change is to show the current date in the first month position, not the second. Since the user will be selecting past dates, it'd be easier to show more past dates, than future dates in the calendar.

For the Chart Filter, the button label still has the icon, which could be removed (see screenshots)

The icons used in the chart type select are more critical. I don't think those should be removed. They reinforce chart types, layouts, etc.

kappa90 · 2022-07-07T18:50:14Z

The date range select component doesn't show the active border color when focused. The border should show active blue when the field is focused. It looks like this might be affecting other selects in this context in your screenshots.

Done, the reason is that this is an antd popup, but it appears only here:

It looks like item 4 from #10640 isn't included. The main change is to show the current date in the first month position, not the second. Since the user will be selecting past dates, it'd be easier to show more past dates, than future dates in the calendar.

Sorry, I think I didn't explain myself correctly. Since that dropdown is from antd, there's limited configuration options, and item #4 on #10640 is not achievable unless we rewrite the whole calendar picker ourselves.

clarkus

I would really like to find a way to optimize the calendar picker for selecting dates in the past, but don't want to block this improvement based on that alone.

kappa90 · 2022-07-07T18:55:04Z

I would really like to find a way to optimize the calendar picker for selecting dates in the past, but don't want to block this improvement based on that alone.

How about we merge these and I release the new date filter component, and then I open another PR to improve the calendar picker?

clarkus · 2022-07-07T18:57:57Z

Yup I'm a +1 on shipping it.

mariusandra

Few comments (plus the tests, which I see you're fixing)

mariusandra · 2022-07-08T08:06:36Z

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

+
+.ant-picker-range-arrow {
+    border-top: 1px solid var(--primary);
+    border-right: 1px solid var(--primary);
+}
+.ant-picker-range-wrapper {
+    border: 1px solid var(--primary);
+}


These styles will apply to all such classes everywhere, not just this date picker. This may have some effect outside of just this component, I'm not sure.

This is basically adding a blue border to the antd date calendar popup, which makes it style-coherent with our Lemon popups.

The next step would be to remove that component and create a LemonCalendarPicker, which I'm going to do after.

Is there a better place where to move these classes?

Just sanity checking:
Will it show when the experiment is off? Do we use some other date pickers elsewhere in the app?

Only in the DatePicker because the class is imported locally.

mariusandra · 2022-07-08T08:08:25Z

frontend/src/lib/utils.test.ts

@@ -271,23 +271,23 @@ describe('dateFilterToText()', () => {
            const from = dayjs('2018-04-04T16:00:00.000Z')
            const to = dayjs('2018-04-09T15:05:00.000Z')

-            expect(dateFilterToText(from, to, 'custom', dateMapping, true)).toEqual('4 Apr 2018 - 9 Apr 2018')
+            expect(dateFilterToText(from, to, 'custom', dateMapping, true)).toEqual('April 4 - April 9, 2018')


If the year changes (e.g. april 4 2016 - april 9 2018), will it show both years?

It does and I added a test

mariusandra · 2022-07-08T08:09:02Z

frontend/src/lib/utils.tsx

-const DATE_FORMAT = 'D MMM YYYY'
+const DATE_FORMAT = 'MMMM D, YYYY'
+const DATE_FORMAT_WITHOUT_YEAR = 'MMMM D'


Where does this change the dates? Just the date picker, or other random places in the app?

I checked. Just in the Date Filter for now, unless you use dateFilterToText with isFormatted=true, which is not used anywhere else.

frontend/src/scenes/dashboard/DashboardHeader.tsx

Co-authored-by: Marius Andra <marius.andra@gmail.com>

chore: date filter UI fixes

177dac7

kappa90 requested review from mariusandra, clarkus, pauldambra, benjackwhite and Twixes July 7, 2022 17:14

fix eslint errors

44fd184

clarkus suggested changes Jul 7, 2022

View reviewed changes

fixes

7a20281

kappa90 requested a review from clarkus July 7, 2022 18:50

clarkus approved these changes Jul 7, 2022

View reviewed changes

fix test

f4485f8

kappa90 removed the request for review from Twixes July 7, 2022 19:21

fix cypress

cd22c9b

mariusandra reviewed Jul 8, 2022

View reviewed changes

kappa90 and others added 2 commits July 8, 2022 09:23

add test

1cf3c43

Update frontend/src/scenes/dashboard/DashboardHeader.tsx

662fad6

Co-authored-by: Marius Andra <marius.andra@gmail.com>

mariusandra approved these changes Jul 8, 2022

View reviewed changes

kappa90 merged commit a1c0aaf into master Jul 8, 2022

kappa90 deleted the chore/date-filter-ui-fixes branch July 8, 2022 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: date filter UI fixes #10673

chore: date filter UI fixes #10673

kappa90 commented Jul 7, 2022 •

edited

Loading

clarkus left a comment

kappa90 commented Jul 7, 2022

clarkus left a comment

kappa90 commented Jul 7, 2022

clarkus commented Jul 7, 2022

mariusandra left a comment

mariusandra Jul 8, 2022

kappa90 Jul 8, 2022

mariusandra Jul 8, 2022

kappa90 Jul 8, 2022

mariusandra Jul 8, 2022

kappa90 Jul 8, 2022

mariusandra Jul 8, 2022

kappa90 Jul 8, 2022

chore: date filter UI fixes #10673

chore: date filter UI fixes #10673

Conversation

kappa90 commented Jul 7, 2022 • edited Loading

Problem

Changes

How did you test this code?

clarkus left a comment

Choose a reason for hiding this comment

kappa90 commented Jul 7, 2022

clarkus left a comment

Choose a reason for hiding this comment

kappa90 commented Jul 7, 2022

clarkus commented Jul 7, 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

Choose a reason for hiding this comment

kappa90 commented Jul 7, 2022 •

edited

Loading