Add TimeRangePicker component #206

sashathor · 2024-03-28T11:53:15Z

Description of changes

This PR implements Add Date & Time Picker component and syncs all the UI components with new design tokens introduced in #165.

New components:

Checklist

Before merging to main:

Tests
Manually tested in React apps
Changesets
Approved

changeset-bot · 2024-03-28T11:53:19Z

🦋 Changeset detected

Latest commit: ce41b55

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@propeldata/ui-kit	Minor
dashboard-example	Major
react-16	Patch
react-18	Patch
react-17	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2024-03-28T11:53:21Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
ui-kit	⬜️ Ignored (Inspect)	Visit Preview		May 21, 2024 11:52am

… examples Dashboard

felipecadavid

The component looks amazing! I left a few comments

Also, we've added a lot of primitive components (Button, Select, etc) in this PR, I'm thinking about not mixing those up in the components directory, how about having a primitives directory for those components?

Please also let's move the stories from having the 'pattern' tag to the devOnly array in manager.ts so those are visible in dev

app/storybook/.storybook/preview.tsx

packages/examples/dashboard/src/components/CounterConnected/CounterConnected.tsx

packages/examples/dashboard/src/components/LeaderboardConnected/LeaderboardConnected.tsx

packages/ui-kit/package.json

packages/ui-kit/src/components/TimeRangePicker/DateTimeField/DateTimeField.tsx

packages/ui-kit/src/components/TimeRangePicker/TimeRangePicker.tsx

packages/ui-kit/src/components/TimeRangePicker/TimeRangePicker.types.ts

packages/ui-kit/src/components/TimeRangePicker/TimeRangePicker.tsx

markandrus

Looking great! Left some questions/suggestions about prop names.

Also, suggest to use en dashes (–) and true ellipses (…) everywhere we can, rather than hyphens (-) and periods (...). Can you check that we do?

Finally, will spend some time playing with this in Storybook.

app/storybook/.storybook/manager.ts

packages/ui-kit/src/components/Divider/Divider.tsx

packages/ui-kit/src/components/TimeRangePicker/TimeRangePicker.tsx

markandrus · 2024-05-08T11:51:58Z

packages/ui-kit/src/components/TimeRangePicker/TimeRangePicker.tsx

+            ? `${formatDateTime(datepickerRange?.from, locale)} - ${formatDateTime(datepickerRange?.to, locale)}`
+            : `${formatDateTime(datepickerRange?.from, locale)} - Now`,


I have seen there is also a formatRange API for locale-aware range formatting. I think we don't need to tackle that right now (or ever?). But it is interesting: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/formatRange

Main benefit seems to be for representing ranges when ranges share the same day, the same month, etc. (e.g., where to place the hyphen).

packages/ui-kit/src/components/TimeRangePicker/TimeRangePicker.tsx

packages/ui-kit/src/components/TimeRangePicker/TimeRangePicker.types.ts

markandrus

1. Tabbing behavior: I can tab to the dropdown to select it and hit enter/space to open it. But then, when I tab again, it jumps straight to the "In the last" option's numeric input, skipping over "Today", "This week", etc. I would expect to tab through those options first, no? EDIT: I just tried Radix's Dropdown Menu, and it seems like this may be expected? So maybe this behavior is good?
2. Tabbing/keyboard behavior (numeric input): Same as above, I tab into the "In the last" option's numeric input, but then the keyboard up/down keys do not increment/decrement the input. They instead move my selection around.
3. Tabbing/keyboard behavior (days/weeks/months dropdown): Same as above, I tab into the "In the last" option's numeric input, then tab again to the days/weeks/months dropdown, and hit enter/space to open it. But then the keyboard up/down keys do not move within the days/weeks/months dropdown. They instead move my selection around in the outer dropdown.
4. Keyboard navigation (escape key): I would expect pressing escape to close the menu, wherever I am (can see this behavior in Radix, too).
5. From custom date until now…: Should I be able to click within the range (not just outside the range) to shorten it? Currently, I cannot click "6" for example to change the start date.
6. Notes on text/copy: Something to discuss with @acossta but,
- Why is it "Custom fixed date range"? Why is "fixed" significant? IMO, it should just be "Custom date range".
- Why does "From custom date until now…" end in ellipsis? Why not the other option? IMO, we should get rid of the ellipsis.
- Why emphasize "custom date"? It's more like, "From a specific date until now" or "From a particular date until now" or just "From date until now". IMO, we should do "From date until now".
If we agree, that would leave us with just "From date until now" and "Custom date range".
- ✅ Quick discussion in front-end sync today: we decided to go with "From date until now" and "Custom date range".
7. Apply behavior: I haven't applied anything yet, but clicking on a date to deselect it will update the dropdown label ("- Now"). IMO, it should continue to show what was previously selected. Additionally, the "Apply" button should be disabled until I select a valid date range. This happens on both "From date until now" and "Custom date range".
8. Start/end bar being hidden: IMO, we should rethink whether the start/end bar should be hidden when a date range is not selected. It leads to the whole modal shifting, which is something to avoid, no? IMO, it would be better to just show something like "Start: — End: —" if no values are available yet. This applies to both "From date until now" and "Custom date range".
- ✅ Quick discussion in front-end sync today: we decided to keep this bar always visible with some placeholder, to avoid shifting.
9. No "back" behavior?: We have "Cancel" button and "x" icon for closing the pickers, but I feel like I'd want to go back to the previous menu instead. 🤔 Like, if I click "Cancel", I'd want to just go back to the menu (maybe the button should even be "Back"?). Also, I'm not even sure if an "x" icon is necessary. We don't have an "x" icon on the initial menu. Why have it on the next one? IMO, we should get rid of it.
- ✅ Quick discussion in front-end sync today: we decided to remove the "x" icon, and to let "Cancel" take you back to the preceding menu.

acossta · 2024-05-09T18:22:12Z

The component looks great. I'm worried about the additional dependency on Google fonts.

A couple of questions:

Can we ship this with a font that doesn't require that, like Helvetica or Arial?
Ideally, customizing the font would be done at the theme level. If you need Inter or any other Google font, it would be expected that you have to install additional dependencies. Would this be a feasible approach?

sashathor · 2024-05-13T14:40:16Z

The component looks great. I'm worried about the additional dependency on Google fonts.

A couple of questions:

Can we ship this with a font that doesn't require that, like Helvetica or Arial?

Ideally, customizing the font would be done at the theme level. If you need Inter or any other Google font, it would be expected that you have to install additional dependencies. Would this be a feasible approach?

@acossta I've added sans-serif as a fallback.

So, before the change and without Inter installed we had the following:

After the change and without Inter installed we will have:

With Inter installed:

markandrus · 2024-05-13T14:51:54Z

Some more things I noticed

1. "From date until now": Choose this, apply a date range, re-open the menu, choose this again, notice the previously chosen date range is no longer visible. If you do the same flow with "Custom date range", it's not reset. I.e., you can see the value you previously applied. IMO, it should remember the value that was last set, and the two options should behave the same.
2. Keyboard navigation: If you tab into the numeric input, increment or decrement it, and then tab next to the days/weeks/months dropdown (because you want to see the time unit now), the outermost menu quickly closes. IMO, so long as an input/submenu is focused in the menu, the outermost menu should not close.
3. Keyboard navigation: Tabbing out of the days/weeks/months dropdown does not close it.
4. Clearing: clearing a cleared date range leaves the button focused.
5. Closing: can't close the outermost menu via escape key or clicking outside of it. Within the inner date range pickers, escape key and clicking outside work to close the whole menu.

sashathor · 2024-05-14T13:50:53Z

Keyboard navigation: If you tab into the numeric input, increment or decrement it, and then tab next to the days/weeks/months dropdown (because you want to see the time unit now), the outermost menu quickly closes. IMO, so long as an input/submenu is focused in the menu, the outermost menu should not close.

@markandrus I followed Posthog's picker behavior. We can re-think it later if needed. Wdyt?

markandrus · 2024-05-14T14:48:59Z

@sashathor sounds good for now!

sashathor · 2024-05-14T15:22:19Z

@markandrus found a few more bugs. I'll let you know when the PR will be ready for the next round.

felipecadavid

I left a few comments on the code, I think it's getting there

packages/ui-kit/src/components/TimeRangePicker/TimeRangePicker.tsx

packages/ui-kit/src/components/Select/Select.tsx

felipecadavid · 2024-05-16T22:05:05Z

It seems like I cannot close the select by clicking back on it? it seems like it closes but quickly opens again. The behavior is a bit weird as I can tell sometimes it actually closes

Screen.Recording.2024-05-16.at.5.02.52.PM.mov

felipecadavid · 2024-05-20T23:25:15Z

packages/ui-kit/src/helpers/localeDateTime/localeDateTime.ts

+      switch (part.type) {
+        case 'year':
+          return 'yyyy'
+        case 'month':
+          return options?.month === 'long' ? 'MMM' : 'MM'
+        case 'day':
+          return 'dd'
+        case 'hour':
+          return is12HourFormat(locale) ? 'hh' : 'HH'
+        case 'minute':
+          return 'mm'
+        case 'second':
+          return 'ss'
+        case 'dayPeriod':
+          return is12HourFormat(locale) ? 'a' : ''
+        case 'literal':
+          // Replace non-breaking space with regular space
+          return part.value.charCodeAt(0) === 8239 ? ' ' : part.value
+        default:
+          return part.type
+      }


I think an object literal makes more sense here

{ year: 'yyyy', month: options?.month === 'long' ? 'MMM' : 'MM', day: 'dd' ... }[part.type]

Is there something wrong with switch?

Usually an object literal brings a better readabilty, performance (evaluating multiple cases vs mapping) and maintainability. In some cases you may want to still use a switch for convenience e.g complex conditions or adding additional logic to each case, but I think for this case the object literal works much better

vercel bot had a problem deploying to Preview March 28, 2024 11:54 Failure

sashathor changed the title ~~Sasha/pro 2111 add date time picker component~~ WIP: Add TimePicker component Mar 28, 2024

vercel bot deployed to Preview March 28, 2024 12:26 View deployment

sashathor force-pushed the sasha/pro-2111-add-date-time-picker-component branch from bc060e8 to 5a04145 Compare March 28, 2024 12:43

vercel bot had a problem deploying to Preview March 28, 2024 12:43 Failure

vercel bot deployed to Preview March 28, 2024 12:49 View deployment

sashathor mentioned this pull request Apr 5, 2024

feat(ui-kit): add Design Tokens synchronization automation from Figma to UI Kit #165

Merged

4 tasks

sashathor added 4 commits April 8, 2024 09:59

chore(ui-kit): temp

629aa45

feat(ui-kit): improvements for TimePicker and releated components

e9ba7e6

chore(ui-kit): updates

cea296e

chore(deps): update lock files

28c3b93

sashathor force-pushed the sasha/pro-2111-add-date-time-picker-component branch from 981dfd5 to 28c3b93 Compare April 8, 2024 08:08

vercel bot had a problem deploying to Preview April 8, 2024 08:09 Failure

chore(deps): update lock files

373caba

vercel bot deployed to Preview April 8, 2024 08:15 View deployment

chore(time-picker): sync with new design tokens

627254a

sashathor changed the title ~~WIP: Add TimePicker component~~ Add TimePicker component Apr 17, 2024

sashathor added 4 commits April 18, 2024 11:38

chore(ui-kit): add tests for TimeRangePicker

34cc434

chore(storybook): add docs for TimeRangePicker

443ca53

chore(cicd): add changesets

d91d97f

fix(ui-kit): TimeRangePicker minor fixes

def80cc

sashathor marked this pull request as ready for review April 18, 2024 14:06

sashathor added 2 commits April 18, 2024 16:26

fix(ui-kit): fix TimeRangePicker tests

c12a306

chore(ui-kit,examples): TimeRangePicker fixes, add TimeRangePicker to…

1541a12

… examples Dashboard

sashathor requested review from felipecadavid and markandrus April 22, 2024 13:49

felipecadavid reviewed Apr 25, 2024

View reviewed changes

sashathor changed the title ~~Add TimePicker component~~ Add TimeRangePicker component Apr 26, 2024

sashathor added 3 commits May 2, 2024 18:13

chore(ui-kit, storybook): updates

7019680

chore(ui-kit): TimeRangePicker pr comment updates

a92a21f

chore(ui-kit): TimeRangePicker pr comment updates

48aa57c

sashathor requested review from markandrus and felipecadavid May 7, 2024 11:06

vercel bot deployed to Preview May 7, 2024 11:10 View deployment

markandrus reviewed May 8, 2024

View reviewed changes

chore(ui-kit): TimeRangePicker pr comment updates

91bae3a

sashathor added 2 commits May 14, 2024 15:18

chore(ui-kit): TimeRangePicker pr comment updates

25abea0

fix(ui-kit): build fix

f8ca95c

sashathor requested a review from markandrus May 14, 2024 13:50

sashathor added 2 commits May 16, 2024 13:44

fix(ui-kit): fix nested Selects

9825f93

fix(ui-kit): fix TimeRangePicker

838e832

felipecadavid reviewed May 16, 2024

View reviewed changes

chore(ui-kit): TimeRangePicker pr comment updates

52cc607

sashathor requested a review from felipecadavid May 17, 2024 13:40

fix(ui-kit): fix Select multiple clicks

fb786ee

felipecadavid reviewed May 20, 2024

View reviewed changes

fix(ui-kit): fix Select multiple clicks

ce41b55

felipecadavid approved these changes May 21, 2024

View reviewed changes

sashathor merged commit ebdd0da into main May 21, 2024
3 checks passed

sashathor deleted the sasha/pro-2111-add-date-time-picker-component branch May 21, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TimeRangePicker component #206

Add TimeRangePicker component #206

sashathor commented Mar 28, 2024 •

edited

Loading

changeset-bot bot commented Mar 28, 2024 •

edited

Loading

vercel bot commented Mar 28, 2024 •

edited

Loading

felipecadavid left a comment

markandrus left a comment

markandrus May 8, 2024

markandrus left a comment •

edited by sashathor

Loading

acossta commented May 9, 2024

sashathor commented May 13, 2024

markandrus commented May 13, 2024 •

edited by sashathor

Loading

sashathor commented May 14, 2024

markandrus commented May 14, 2024

sashathor commented May 14, 2024

felipecadavid left a comment

felipecadavid commented May 16, 2024

felipecadavid May 20, 2024

sashathor May 21, 2024

felipecadavid May 21, 2024

		? `${formatDateTime(datepickerRange?.from, locale)} - ${formatDateTime(datepickerRange?.to, locale)}`
		: `${formatDateTime(datepickerRange?.from, locale)} - Now`,

Add TimeRangePicker component #206

Add TimeRangePicker component #206

Conversation

sashathor commented Mar 28, 2024 • edited Loading

Description of changes

Checklist

changeset-bot bot commented Mar 28, 2024 • edited Loading

🦋 Changeset detected

vercel bot commented Mar 28, 2024 • edited Loading

felipecadavid left a comment

Choose a reason for hiding this comment

markandrus left a comment

Choose a reason for hiding this comment

markandrus May 8, 2024

Choose a reason for hiding this comment

markandrus left a comment • edited by sashathor Loading

Choose a reason for hiding this comment

acossta commented May 9, 2024

sashathor commented May 13, 2024

markandrus commented May 13, 2024 • edited by sashathor Loading

sashathor commented May 14, 2024

markandrus commented May 14, 2024

sashathor commented May 14, 2024

felipecadavid left a comment

Choose a reason for hiding this comment

felipecadavid commented May 16, 2024

felipecadavid May 20, 2024

Choose a reason for hiding this comment

sashathor May 21, 2024

Choose a reason for hiding this comment

felipecadavid May 21, 2024

Choose a reason for hiding this comment

sashathor commented Mar 28, 2024 •

edited

Loading

changeset-bot bot commented Mar 28, 2024 •

edited

Loading

vercel bot commented Mar 28, 2024 •

edited

Loading

markandrus left a comment •

edited by sashathor

Loading

markandrus commented May 13, 2024 •

edited by sashathor

Loading