Conversation
packages/gamut/src/DatePicker/DatePickerInput/Segment/DatePickerInputSegment.tsx
Show resolved
Hide resolved
dreamwasp
left a comment
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 ☕
| @@ -0,0 +1,123 @@ | |||
| /** | |||
There was a problem hiding this comment.
nit: i really prefer object syntax for function arg props for stuff like this so they can't get mixed up
| @@ -0,0 +1,77 @@ | |||
| import type { IsoWeekday } from '../../utils/locale'; | |||
| import { stringifyLocale } from '../../utils/locale'; | |||
|
|
|||
There was a problem hiding this comment.
same comment of obj arg stucture
| // 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
| range && isDateInRange(date, selectedDate, endDate); | ||
| const disabled = isDateDisabled(date, disabledDates); | ||
| const today = isToday(date); | ||
| // this is making the selected date a differnet color bc it is focused, look into further |
| 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
| const segmentStyles = states({ | ||
| default: { | ||
| color: 'text-secondary', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
this seems weird - can this just be a css style
There was a problem hiding this comment.
default is a terrible descriptor but the text color changes when its the placeholder MM/DD/YYYY vs when you have typed or selected and there's an actual date. but yes i can do this in css too
| * Shell uses the same style stack as `Input`. `formFieldStyles` targets `&:focus`, but the host is a | ||
| * `div` — focus is on inner spinbuttons, so we mirror `Input` focus visuals with `&:focus-within`. | ||
| */ | ||
| export const SegmentedShell = styled(FlexBox)<SegmentedShellProps>( |
There was a problem hiding this comment.
this could be done with a state instead of css
|
📬 Published Alpha Packages:
|
|
🚀 Styleguide deploy preview ready! Preview URL: https://69dd4273621b22183ebaaa69--gamut-preview.netlify.app |
sh0ji
left a comment
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.
| placeholder?: string; | ||
| /** Override UI strings (e.g. clear button). Merged with defaults. */ | ||
| translations?: DatePickerTranslations; | ||
| inputSize?: ComponentProps<typeof Input>['size']; |
There was a problem hiding this comment.
I've never used this prop borrowing syntax. will it carry the comments over from
in type-aware contexts like IDEs? if so, let's add a description there. if not, we need one here to explain the role of this prop.There was a problem hiding this comment.
looks like it shows the prop comments if i hover over size but not directly for inputSize. will add a comment
| /** 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?
| /** Start date (range) or selected date (single). */ | ||
| startOrSelectedDate: Date | null; |
There was a problem hiding this comment.
I know we discussed the awkwardness of this but it's especially evident in this one prop. I like how you resolved it for the DatePickerProps discriminated union. is there any reason we couldn't do that for the context as well?
might even be able to make it generic to determine the right props (e.g., useDatePicker<Mode extends 'single' | 'range' | undefined>()) rather than trying to unify them, but that might not be necessary.
There was a problem hiding this comment.
we can and i tried it at one point, but then we're going to have a bunch more conditionals and branches for single vs range whereas this seemed cleaner to me. i can try it again now that the code is a little more settled
| /** 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
| import { CalendarBaseProps, QuickAction } from './Calendar/types'; | ||
| import { DatePickerTranslations } from './utils/translations'; | ||
|
|
||
| export interface DatePickerBaseProps |
There was a problem hiding this comment.
big semantics nit, but I think mode should be part of DatePickerBaseProps. by re-declaring it on both DatePickerSingleProps & DatePickerRangeProps, it's not guaranteed to be recognize as the same prop, even though they're named the same. this may or may not affect inference in some edge cases.
export interface DatePickerBaseProps<Mode extends 'single' | 'range' | undefined>
extends Pick<CalendarBaseProps, 'locale' | 'disabledDates'> {
mode: Mode;
...
}
export interface DatePickerSingleProps extends DatePickerBaseProps<'single' | undefined> {
/* don't declare mode here */
...
}
export interface DatePickerRangeProps extends DatePickerBaseProps<'range'> {
/* don't declare mode here */
...
}There was a problem hiding this comment.
oh also, I don't think DatePickerBaseProps should be exported. it feels like an internal type—the public types are DatePickerSingleProps, DatePickerRangeProps, and DatePickerProps.
| endLabel?: string; | ||
| } | ||
|
|
||
| export type DatePickerProps = DatePickerSingleProps | DatePickerRangeProps; |
There was a problem hiding this comment.
I quite like the way you compose these types together! it results in a great type experience where my IDE can infer that when mode = 'single', the startDate prop is invalid. nicely done.
@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) |
5 participants
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