-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: DatePicker component #803
Conversation
As noted in the PR description, I've encountered a blocker to finishing this, which is that React 17 is required in order to implement the In the meantime, any feedback on the general structure of this component is appreciated! My biggest open question is whether it's worth converting any of the existing state hooks to |
- Keep React 16 peerDependency for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good so far 😇, thanks for keeping the hooks organized that helps a lot. I didn't run yet just cuz I know things in progress .
Will be interested to see what ends up being similar to Combobox or not. Originally I was thinking we could share some logic but I can see how different they are when I look closer.
Made some comments about stuff to consider.
DAY_OF_WEEK_LABELS, | ||
DAY_OF_WEEK_SHORT_LABELS, | ||
MONTH_LABELS, | ||
} from './constants' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please surface these as optional props for i18n purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue added: #866
'You can navigate by day using left and right arrows', | ||
'Weeks by using up and down arrows', | ||
'Months by using page up and page down keys', | ||
'Years by using shift plus page up and shift plus page down', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 More lang that is not i18n friendly
|
||
/** | ||
* This file contains the USWDS DatePicker date manipulation functions converted to TypeScript | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I get that we are just doing this for speed and copy/pasting USWDS tests but I'm a bit weirded out that this doesn't have 100% unit tests coverage for every function for maintenance reasons. Maybe we should backlog a story to either test everything here or else swap with an external dependency (some date/time lib) that would already be well tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USWDS didn't seem to have tests for these methods at all. I started writing some but I haven't gotten them all.. I'm hoping to get as much coverage on these as possible but some of them are just tricky because dates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow. 🥇 Okay well then thank you for starting on this, I misunderstood! Do you not want to lean on a lightweight date/time lib or util? I can see why we wouldn't want to do that just for this component... but just checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I thought about it, but it would mean importing a new lib and I'm making the assumption that USWDS wrote their own utils to keep things lightweight and for the specific use cases required based on how they decided to architect the DatePicker. I think I want to just follow their decision on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also occurred to me later on, but if USWDS makes updates to their DatePicker code then it will be harder to reconcile differences if we deviate from them.
…picker-component-338
@haworku I'm not seeing either of these issues myself! I've since reconciled this branch with the latest Also I think you have a good point about not exporting stories for components that are not meant to be used externally. They're super useful for development, but are probably confusing if included in our published Storybook. I will comment those out! |
@suzubara ✅ I see everything now. I cleared out node modules again something must have been in a really funky state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅
small housekeeping things were:
- comment out stories for components that aren't exported to reduce confusion
- backlog an issue if there are any outstanding validations bugs (code comments suggest there are?). I couldn't find any in terms of manual testing.
/* | ||
OUTSTANDING: | ||
- focus out on date picker external input validates | ||
- keydown on date picker input validates if keycode is enter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these validation tasks still outstanding? If so lets file bugs for them. I think one of these has a skipped test later on in the file, seems like we could remove it for now from here and add to the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just a few questions, but otherwise this looks amazing! I know a lot of work went into this. Awesome job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good from my end. Great work!
Summary
This PR adds the USWDS DatePicker component.
I worked off of the USWDS DatePicker source code and attempted to re-implement as closely to that spec as possible using React paradigms and features where appropriate. The
constants.ts
andutils.ts
are almost exact copies of the constants and utility methods from the source, other than being converted to TypeScript.The exported component and its props interface are:
This renders an internal hidden text
<input />
and an external visible text<input />
, a button for toggling the date picker dialog, and hidden status text for screen readers. As described in the USWDS spec, dates passed into any of theprops
must use the formatYYYY-MM-DD
and a date typed directly into the external input must use the formatMM/DD/YYYY
.When opening the date picker dialog, the
<Calendar />
subcomponent is displayed and can be used to select a date from the current month, navigate to previous & next months and years, as well as select specific months and years by clicking on the currently selected ones (which toggles display of the<MonthPicker />
and<YearPicker />
subcomponents).The
minDate
prop defaults to01-01-0000
and there is no defaultmaxDate
. If a min and/or max are specified, dates outside of that range become disabled in the Calendar UI, and if an invalid date is typed into the input the input itself becomes invalid. No error UI is implemented - this is up to consumers to handle.Note: There is one small difference in functionality between this component and the USWDS implementation, related to validating the input. The USWDS implementation validates when:
Because this component uses the
useEffect
hook to trigger validation whenever the date value changes (regardless of how), the React DatePicker will validate when:This is a minor discrepancy but one still worth noting, so I am adding it to the Storybook docs for the component. It's also worth mentioning that validation in this case is just calling
setCustomValidity
on the external text input, and library users should be able to determine how & when they want invalid UI to display by inspecting the ValidityState of the external input. We may find that we want to expose props for custom event handlers or even a ref to the component for better integration with 3rd party form libraries, but I would recommend leaving that for a future story once we have a better understanding of how this component will be used.Related Issues or PRs
Fixes #338
How To Test
I recommend starting Storybook and viewing the Date picker stories. The unit tests should cover all expected interaction and functionality, so reading through those and reproducing them manually in the browser is also a good way to verify that it works as expected.
Subcomponent stories (such as those under Date picker / Calendar, Date picker Day, etc.) are for testing visual cases only and may not work as expected without the context of the full Date Picker.
There are several different cases to test:
rangeDate
prop and the currently selected date. this prop will be used when implementing the Date Range Picker)Screenshots
Calendar hidden (default state on mount)
Calendar open
Month picker
Year picker