Replace date time picker #9909
Replace date time picker #9909
Conversation
|
| : initialRoundedEndDate.toISOString(), | ||
| }); | ||
| if (event) return; | ||
| setAttributeDiff(initializeEventTimes()); |
There was a problem hiding this comment.
A comment would have sped up understanding of this code for me. :-)
| setAttributeDiff(initializeEventTimes()); | |
| // if we're creating a new event, initialize start/end date | |
| setAttributeDiff(initializeEventTimes()); |
There was a problem hiding this comment.
(Would still be helpful if we'd go for the param check as described earlier.)
front/app/containers/Admin/projects/project/events/components/DateTimeSelection/TimeInput.tsx
Show resolved
Hide resolved
| roundToNearestMultipleOfFive, | ||
| calculateRoundedEndDate, |
There was a problem hiding this comment.
These functions are not used anymore now, so they can be removed.
There was a problem hiding this comment.
Would this be a component we want to reuse in different places? If we do, should we put it in a different folder? And then it would need to be slightly changed (use a generic onChange handler that doesn't require a state setter to be passed).
| }; | ||
|
|
||
| return ( | ||
| <Box display="flex" flexDirection="column" maxWidth="400px"> |
There was a problem hiding this comment.
I think maxWidth should rather be defined where this component is imported/used. Reason: it is clearer if all such page-specific styles are defined at the page level, so you don't need to dive inside components to fix issues.
Also, if the plan for this component is to be reusable, maxWidth should rather not live here.
| onChange: (newDate: Date) => void; | ||
| } | ||
|
|
||
| const TimeInput = ({ selectedDate, onChange }: Props) => { |
There was a problem hiding this comment.
I think it's good that all new things we build are accessible out of the box (use semantic HTML when possible). This is a textbook select component. It will help prevent bugs (such as the esc key bug described earlier) and accessibility bugs (the current solution is not keyboard navigable and reads out all 96 options on focusing the options menu, and has possibly more problems).
Because this will likely only be used in the admin, and we're not compliant with a11y there, it's not blocking for this PR.
There was a problem hiding this comment.
I understand your point, but the HTML select is too limited in terms of what styles you can apply. For the same reason, libraries like react-select don't use it either. And since a11y is indeed not an issue for the admin screen I will skip the keyboard navigation issues for now (except for the escape one, that one is really strange)
There was a problem hiding this comment.
We should upgrade our Select component so we can more easily style it. But we're already applying custom Select styling it in several places already. If that's what you mean. :)
There was a problem hiding this comment.
front/app/containers/Admin/projects/project/events/components/DateTimeSelection/TimeInput.tsx
Show resolved
Hide resolved
front/app/containers/Admin/projects/project/events/components/DateTimeSelection/index.tsx
Show resolved
Hide resolved
… new event or editing an existing one
…_review-review Replace date time picker review review
…_widget.cy.ts Co-authored-by: Brent Guffens <[email protected]>
…r_review Replace date time picker: review changes
|
@luucvanderzee Just FYI, this PR still has some unresolved issues. |
Changelog
Changed
Technical
react-datepickerdependency