Conversation
dreamwasp
left a comment
There was a problem hiding this comment.
i also had an issue opening the Calendar with the keyboard, but everything else looks + sounds good on VO.
i'd like to see some form integration tests (Gamut DatePicker / calendar in a form and assert submitted (or controlled) field data). i will prob look over this again tomorrow once i've let it percolate a little bit more ☕
| // eslint-disable-next-line jsx-a11y/control-has-associated-label | ||
| <td | ||
| // fix this error | ||
| // eslint-disable-next-line react/no-array-index-key, jsx-a11y/control-has-associated-label |
There was a problem hiding this comment.
curious about the a11y error here
| import { MiniChevronLeftIcon } from '@codecademy/gamut-icons'; | ||
| import * as React from 'react'; | ||
|
|
||
| import { IconButton } from '../../Button'; | ||
| import { useResolvedLocale } from '../utils/locale'; | ||
| import { CalendarNavProps } from './types'; | ||
| import { getRelativeMonthLabels } from './utils/format'; | ||
|
|
||
| export const CalendarNavLastMonth: React.FC<CalendarNavProps> = ({ | ||
| displayDate, | ||
| onDisplayDateChange, | ||
| onLastMonthClick, | ||
| locale, | ||
| }) => { | ||
| const resolvedLocale = useResolvedLocale(locale); | ||
| const { lastMonth } = getRelativeMonthLabels(resolvedLocale); | ||
|
|
||
| const handleLastMonth = () => { | ||
| const lastMonth = new Date( | ||
| displayDate.getFullYear(), | ||
| displayDate.getMonth() - 1, | ||
| 1 | ||
| ); | ||
| onDisplayDateChange?.(lastMonth); | ||
| onLastMonthClick?.(); | ||
| }; | ||
|
|
||
| return ( | ||
| <IconButton | ||
| alignSelf="flex-start" | ||
| aria-label={lastMonth} | ||
| icon={MiniChevronLeftIcon} | ||
| size="small" | ||
| tip={lastMonth} | ||
| onClick={handleLastMonth} | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
i feel like CalendarNavLastMonth + NextMonths could be one component + DRYED up
There was a problem hiding this comment.
yeah ive been fussing with this a bunch. do you think the keyboard nav/tab order makes sense? its something like left arrow left calendar body right arrow. or should it be left arrow right arrow calendar. i was updating to make the calendar headers more attached to the calendar body in the reading order
|
📬 Published Alpha Packages:
|
|
🚀 Styleguide deploy preview ready! Preview URL: https://69dd4273621b22183ebaaa69--gamut-preview.netlify.app |
sh0ji
left a comment
There was a problem hiding this comment.
heads up that I only really reviewed the API, not the implementation. looks really good in general! but I did have quite a few questions and there are three things that could use refinement:
- every prop needs a description and we should be very picky about those descriptions. prop descriptions are our most important docs since they can clarify usage, help in situations where naming is hard, and because they're our best just-in-time docs since they're visible during development thanks to IDE type hinting.
- naming conventions: I'm of the opinion that callback props should always be named
on${Event}, and that seems to be the case across Gamut. it's possible that some of the props I commented on aren't actually callback props, which would reinforce my first point (better descriptions). - the focus management API feels too big to me. happy to brainstorm ideas to solve it, but my instinct is to remove as much of it as possible and just handle it internally. it's also easier to add props later than it is to remove them after they're out.
| /** Move focus from the input into the grid when the calendar is already open (e.g. ArrowDown). */ | ||
| focusCalendarGrid: () => void; | ||
| /** | ||
| * Flips on each grid focus request so `CalendarBody` effects re-run when `focusTarget` is unchanged. | ||
| * Not a semantic true/false — only the change matters; pair with `gridFocusRequested`. | ||
| */ | ||
| focusGridSignal: boolean; | ||
| /** When true, `CalendarBody` runs a one-shot move of DOM focus into the grid if it is not already there. */ | ||
| gridFocusRequested: boolean; | ||
| /** Clears `gridFocusRequested` after focus has moved into the grid (or call when closing). */ | ||
| clearGridFocusRequest: () => void; |
There was a problem hiding this comment.
these four props plus moveFocusIntoCalendar in OpenCalendarOptions is five props just for focus management, which feels like a huge API for something that I don't personally think developers want to spend that much time thinking about. how much of it do we need to expose to users?
There was a problem hiding this comment.
i dont think any of this needs to be exposed to users
There was a problem hiding this comment.
yeah this is the context props so we're handling all of this internally within DatePicker, this is what is within the context. i can move this type into a different file if thats clearer?
| /** Which input is active (start/end focused); null = selection mode. */ | ||
| activeRangePart: ActiveRangePart; | ||
| /** Set which input is active (e.g. when input receives focus). */ | ||
| setActiveRangePart: (part: ActiveRangePart) => void; |
There was a problem hiding this comment.
I'm confused. "active" = focused? and "part" = the two different inputs (start/end)? the semantics and names of these props could use some refinement. and if these are part of the focus management API, I also wonder if there's just some better way to handle this?
There was a problem hiding this comment.
yes this is for the logic when you specifically click on one of the inputs and then select a date in the calendar
There was a problem hiding this comment.
lmk if you have a better naming suggestion or way to handle this
@sh0ji will update prop descriptions, naming conventions, and clean up what we're exporting (i think this will help with the focus management stuff too) |
LinKCoding
left a comment
There was a problem hiding this comment.
I didn't review the docs fwiw -- but I did test the functionality (not including VO) and it worked well using mouse!
I think my review can be grouped into 3 things:
- Clean-up around naming and comments (comments esp could be part of a later ticket)
- Some RTL clean-up, albeit, you're blocked by current RTL fixes
- Echoing Cass's point about using objects in function parameters/arguments
All in all, looking really good :)
| if (startDate && endDate) { | ||
| // if start date is end date and is clicked, clears everything | ||
| if ( | ||
| startDate.getTime() === endDate.getTime() && |
There was a problem hiding this comment.
purely stylistic, but you could also use isSameDay here
| delta: 1 | -1 | ||
| ) => { | ||
| const { min, max } = getSegmentSpinBounds(field, segments); | ||
| let cur = parseSegmentNumericString(segments[field]); |
There was a problem hiding this comment.
maybe currentSegementVal to be more accurate?
| fieldOrder: DatePartKind[] | ||
| ): SegmentValues => { | ||
| let rest = digits; | ||
| const out: SegmentValues = { month: '', day: '', year: '' }; |
There was a problem hiding this comment.
out => segmentDigits? something more descriptive?
| size={inputSize} | ||
| /> | ||
| <Box alignSelf="center" mt={32}> | ||
| <MiniArrowRightIcon /> |
There was a problem hiding this comment.
Note: could add a TODO comment for later, but this will need to adjust for RTL (waiting for the useElementDir hook in another PR)
| ); | ||
|
|
||
| const setButtonRef = useCallback((date: Date, el: HTMLElement | null) => { | ||
| const k = new Date( |
There was a problem hiding this comment.
leftover robo naming? k => key? dateRef?
Overview
Adds DatePicker to Gamut: a locale-aware, accessible date (or date range) picker with segmented inputs, a popover calendar, keyboard support, and optional composition via context.
Modes
selectedDate/setSelectedDate.startDate,endDate,setStartDate,setEndDate; optionalstartLabel/endLabel.Default UI vs composition
childrenfor layout only; composeDatePickerInputandDatePickerCalendar(calendar requiresDatePickercontext).Segmented inputs
Intl-based layout).Calendar & layout
xsbreakpoint up; one month on smaller viewports.Intl.Locale#getWeekInfo()(polyfill when needed), optionalweekStartsOnonDatePickerCalendar.Selection behavior
Disabled dates
disabledDates— unselectable days; integrated into range validation.Footer
Keyboard & focus
role="spinbutton"span (tabIndex={0}when enabled). Focus moves with Tab / Shift+Tab like normal focusable controls. **Arrow Left / Right ** moves focus within the segments. Arrow Up / Arrow Down steps the current segment up or down, clamped to min/max for that field. Month: 1–12. Day: 1–last day of month when month/year are known. Year: 1–9999; if empty, stepping uses sensible defaults (e.g. current year when stepping up from empty on year).Accessibility
role="dialog"with configurablearia-label.role="group";FormGroupassociates the visible label with the first segment viahtmlFor/id.:focus-withinso the field still shows focus when any inner segment is focused.role="spinbutton", matching Arrow Up/Down stepping and numeric min/max.aria-valuemin/aria-valuemax— match spin bounds (day max depends on month/year when known).aria-valuenow— when there is a numeric value; omitted when empty.aria-valuetext— display string (digits or placeholders likeMM/DD/YYYY).aria-label— field name (month,day,year).aria-invalid— validation/error state.aria-disabledandtabIndex={-1}when disabled.Internationalization
localeusesIntl.LocalesArgument, defaults to runtime locale but ability to override vialocaleproptranslationsfor clear button, field labels, and dialog label. default values in English but ability to override viatranslationspropweekStartsOnuses Intl.Locale#getWeekInfo() (polyfill when needed) but ability to override viaweekStartsOnpropIntl.DateTimeFormatIntl.RelativeTimeFormatOther
inputSizepasses through toInputsizein the default layout.Things I know are missing/not completely working:
PR Checklist
Testing Instructions
Don't make me tap the sign.
PR Links and Envs