-
Notifications
You must be signed in to change notification settings - Fork 5
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
E&A Date Picker (Part One) - Integration and usage within the E&A Timeline header #1044
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
@faustoperez Yeah, I will work on that as we discussed but via this issue: #990. Tried to do it here, but I realized that I might introduce scope creep if I pull it in this sprint (and will make it hard to review the logic in one big PR). |
// We have no reference to the internals of the react-uswds DatePicker, so we have to query for it's trigger. | ||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
if (datePickerRef.current) { | ||
const button = datePickerRef.current.querySelector('button.usa-date-picker__button')! as HTMLButtonElement; |
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.
Ah I see. This is a sticky problem 😬 . Can we control the visibility of the calendar in this way? If we do a conditional render like {showCalendar && <DatePicker...>}
, we will lose the access to this button. (Calendar is not toggling right now, it just re-renders when renderTriggerElement is pressed I think. )
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.
We can't unfortunately, since the <Calendar />
comes coupled with the <DatePicker />
and we can't conditionally render only that component. So the only thing I could think of is programmatically toggling it via the usa-date-picker__button
.
This gets trickier when we will have to show the MONTH_PICKER
or the YEAR_PICKER
based on the dataset time density, because these calendar modes exist as a local state in the Calendar. So the only solution I can think of is to click the picker button to open the calendar, then query and click either usa-date-picker__calendar__month-selection
or usa-date-picker__calendar__year-selection
to toggle the corresponding calendar mode depending on the time density 😰
I opened two issues on react-uswds
side:
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.
Throwing some ideas here. (It might be a terrible idea) Can it be something that we can solve by overwriting the style? I feel like they (calendar trigger element for both uswds date picker and our date picker) are eventually doing the same thing, showing the date, and then a button to trigger the calendar. (I understand our original design wants the whole thing to be a button to trigger - including the text- but maybe we can adjust the design a bit to accommodate USWDS system?)
ex. overwriting the text input style, calendar icon (to show the caret) here?
Maybe we can at least leave the button to trigger and overwrite the style?
Which will be roughly like this - #1047
Orrr 🤔 is this a good component to write React components for our own just using uswds style?
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.
Thanks for the idea @hanbyul-here. As you pointed out, we will need to do a lot of manual triggering if we want to sync the calendar with the time density of the dataset, which I think is very error prone.
is this a good component to write React components for our own just using uswds style
Yes, I think so. We need a more robust approach but at the same time not to reinvent the wheel (e.g. implementing basic date picker functionality from scratch). What do you think about using react-calendar
(https://www.npmjs.com/package/react-calendar)?
Here's a PR with it: #1048 and here's the deployment:
We could override some of the styles using uswds mixins / functions: https://github.com/NASA-IMPACT/veda-ui/pull/1048/files#diff-88b2701907396a34bbe38997d6970f2f6824389dbefe1dcc112a5fdfaa35d811R24
Also:
- It works well with Tippy
- It has out-of-the-box config for showing only the month or year calendar view (it's the
maxDetail
), so it will save us a lot of time
Any opinions 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.
Yeh, I Agree that we should not invent the wheel. I will vote for using ReactCalendar for now 👍
Thanks for making the PR in phases. I think this is a lesson for us that it is not very easy to pick only speicific parts of the components from USWDS. I wonder if what we need is just a Calendar component that the DatePicker component uses. This component is not being exported from the USWDS React library, but it might be a simpler feature request for their end. I can't think of any better approach than how you are doing to control the DatePicker now, but I left a comment on the code about visibility control. There are some items that I noticed:
|
@hanbyul-here Yeah, I think so as well.
Thanks for spotting this, will fix.
Replaced with |
Closing this one as work continued here: #1048 |
**Related Ticket:** _{link related ticket here}_ ### Description of Changes We're introducing a more flexible calendar component which will use some USWDS design tokens and icons. The change is made based on this discussion: #1044 (comment) ### Notes & Questions About Changes _{Add additonal notes and outstanding questions here related to changes in this pull request}_ ### Validation / Testing _{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}_
Related Ticket: #979
Description of Changes
The PR integrates the USWDS date picker into the E&A timeline. To reduce code review complexity, the changes here are:
Next:
Notes & Questions About Changes
I discovered a few limitations of the DatePicker from react-uswds while working on this. For example, if we want to toggle the Calendar via a custom trigger or set a different Calendar mode (day, month, year) based on the temporal density of the datasets, we will have to do it programmatically. The library currently doesn't expose ref, nor props / methods to have more control over the component.
Validation / Testing
{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}