Skip to content

Conversation

@benjaminleonard
Copy link
Contributor

@benjaminleonard benjaminleonard commented Feb 1, 2023

Closes #1132
Closes #1046

First pass at a date picker.

This does not include the quick actions that let you choose a relative time ago, or the multi-month range, though this does not seem complicated to add. Time zone selection could be added on the larger multi-month modal.

Error handling still needs work, and I'm running into some unusual behaviour when the calendar modal closes where it is inconsistent with the element that it focuses on.

This PR is using the react-aria overlay and dialog also. I did not convert it to reach-ui now that it is no longer maintained.

CleanShot 2023-02-01 at 11 00 06

This was a bit of an experiment, but I'd say the react-aria date picker hooks is a good approach. We could for sure get something simpler, a drop-in component, but would be sacrificing a lot of control both on styling and functionality.

@vercel
Copy link

vercel bot commented Feb 1, 2023

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

Name Status Preview Comments Updated
console-ui-storybook ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 16, 2023 at 10:32PM (UTC)

@david-crespo
Copy link
Collaborator

@david-crespo
Copy link
Collaborator

This looks great and we desperately need it.

Bundle size

Bundle size addition is 163 kB / 53 kB gzipped. It's not that much and I don't really see any way around it besides spending time we definitely don't have writing our own 2000 line implementation of date range state. We can always split the pages out like we're doing with Recharts and xterm.js, but honestly even that is of dubious value. The second it causes me some inconvenience I will stop bothering to do it.

Before

image

After

image


export function Calendar(props: CalendarProps<DateValue>) {
const { locale } = useLocale()
const state = useCalendarState({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang lol. Yes, I'm glad we don't have to write that.

{children}
</div>
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you've mentioned this before, but we can almost certainly replace Reach Dialog with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep — goes beyond the scope of this PR but worth discussing a replacement to Reach Dialog at some point now that the library is no longer maintained.

@david-crespo
Copy link
Collaborator

For the record, I tried converting the from 'react-aria' imports to the relevant sub-package and it had zero effect on the resulting bundle size. Was just curious.

diff --git a/package.json b/package.json
index 22fedbed..9ac3f13b 100644
--- a/package.json
+++ b/package.json
@@ -36,6 +36,13 @@
     "@reach/dialog": "^0.16.2",
     "@reach/menu-button": "^0.16.2",
     "@reach/tabs": "^0.16.4",
+    "@react-aria/button": "^3.6.4",
+    "@react-aria/calendar": "^3.0.5",
+    "@react-aria/datepicker": "^3.2.1",
+    "@react-aria/dialog": "^3.4.2",
+    "@react-aria/focus": "^3.10.1",
+    "@react-aria/i18n": "^3.6.3",
+    "@react-aria/overlays": "^3.12.1",
     "@react-spring/web": "^9.5.5",
     "@tanstack/react-query": "^4.12.0",
     "@tanstack/react-query-devtools": "^4.12.0",
@@ -49,7 +56,6 @@
     "mousetrap": "^1.6.5",
     "prop-types": "^15.7.2",
     "react": "^17.0.2",
-    "react-aria": "^3.22.0",
     "react-dom": "^17.0.2",
     "react-error-boundary": "^3.1.3",
     "react-hook-form": "^7.38.0",

@david-crespo
Copy link
Collaborator

Here's what I did:

  • Bring back useButton for calendar button
  • Hook up picker on disk metrics, silo metrics, and system metrics
  • Hook up presets. Picker is only enabled if you choose custom

@david-crespo
Copy link
Collaborator

Haha hell yeah. CI server is of course in UTC. I thought hard coding Chicago in the tests would work, but I forgot there are parts where we pull the local time from the browser. Will fix.

image

@david-crespo
Copy link
Collaborator

Here's a better summary of what's in place. I think it's better than what we have, so I'm ready to merge.

Some issues:

  • Obviously the reset/set buttons are ridiculous
  • When the input is disabled because we're not on "custom", the selected range does not show up on the calendar. Could work around this by disabling the calendar button when the input is disabled, but then you're like "why is this button here?".

2023-02-16-date-picker

})

describe('custom mode', () => {
describe.skip('custom mode', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't get any of this shit to work because getting the displayed date in what used to be a simple <input> is very unsimple now. A lot of this is due to quirks in jsdom. This is one of very few things we're still testing with Testing Library instead of Playwright. I'm inclined to convert these to Playwright, but that can be a followup.

@david-crespo david-crespo merged commit 78f7e9f into main Feb 17, 2023
@david-crespo david-crespo deleted the date-picker branch February 17, 2023 02:37
@benjaminleonard
Copy link
Contributor Author

Here's a better summary of what's in place. I think it's better than what we have, so I'm ready to merge.

Some issues:

  • Obviously the reset/set buttons are ridiculous
  • When the input is disabled because we're not on "custom", the selected range does not show up on the calendar. Could work around this by disabling the calendar button when the input is disabled, but then you're like "why is this button here?".

I think a few of these issues will be resolved with the larger calendar element, since it has the relative times integrated directly in the calendar.

Apply wont be necessary outside of that element because we can apply the input on blur (of the overall input not the individual segments). And by including "Today" in the list of relative time ranges we will no longer need clear.

IMO we move time zone from the inline input but show it in the calendar when opened otherwise it's a million miles long.

I can come back with a PR later, but more than good enough for now. Thanks for getting that integrated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DateTime picker Date Range Component

4 participants