-
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
Improve accessibility for DatePicker #793
Changes from 25 commits
00f70b7
8c02a19
f9c5c0d
9a0be3d
7883b32
53dee9e
cc015d7
955da59
34d816d
5c49239
d6deb63
306a437
dafb6ad
818f4a7
ac86914
6fc900e
54ea538
3004cb7
9ad58cf
12b64cf
9a52857
c46770a
cd624a3
96c7947
3cd8e06
8e2cb90
7f99fd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@vygruppen/spor-react": patch | ||
--- | ||
|
||
Improve accessibility for DatePicker |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
import { | ||
Box, | ||
BoxProps, | ||
FocusLock, | ||
InputGroup, | ||
Popover, | ||
PopoverAnchor, | ||
PopoverArrow, | ||
PopoverBody, | ||
PopoverContent, | ||
PopoverTrigger, | ||
Portal, | ||
ResponsiveValue, | ||
useBreakpointValue, | ||
|
@@ -17,7 +19,12 @@ import { DateValue } from "@internationalized/date"; | |
import { useDatePickerState } from "@react-stately/datepicker"; | ||
import { CalendarOutline24Icon } from "@vygruppen/spor-icon-react"; | ||
import React, { forwardRef, useRef } from "react"; | ||
import { AriaDatePickerProps, I18nProvider, useDatePicker } from "react-aria"; | ||
import { | ||
AriaDatePickerProps, | ||
CalendarProps, | ||
I18nProvider, | ||
useDatePicker, | ||
} from "react-aria"; | ||
import { FormErrorMessage } from ".."; | ||
import { Calendar } from "./Calendar"; | ||
import { CalendarTriggerButton } from "./CalendarTriggerButton"; | ||
|
@@ -30,6 +37,7 @@ type DatePickerProps = AriaDatePickerProps<DateValue> & | |
variant: ResponsiveValue<"simple" | "with-trigger">; | ||
name?: string; | ||
showYearNavigation?: boolean; | ||
withPortal?: boolean; | ||
}; | ||
/** | ||
* A date picker component. | ||
|
@@ -47,6 +55,7 @@ export const DatePicker = forwardRef<HTMLDivElement, DatePickerProps>( | |
errorMessage, | ||
minHeight, | ||
showYearNavigation, | ||
withPortal = true, | ||
width = "auto", | ||
...props | ||
}, | ||
|
@@ -76,33 +85,33 @@ export const DatePicker = forwardRef<HTMLDivElement, DatePickerProps>( | |
ref as React.MutableRefObject<HTMLDivElement> | ||
); | ||
|
||
const styles = useMultiStyleConfig("Datepicker", {}); | ||
const locale = useCurrentLocale(); | ||
|
||
const responsiveVariant = | ||
useBreakpointValue(typeof variant === "string" ? [variant] : variant) ?? | ||
"simple"; | ||
|
||
const locale = useCurrentLocale(); | ||
|
||
const handleEnterClick = (e: React.KeyboardEvent) => { | ||
if ( | ||
responsiveVariant === "simple" && | ||
e.key === "Enter" && | ||
!state.isOpen | ||
) { | ||
// Don't submit the form | ||
e.stopPropagation(); | ||
state.setOpen(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I struggled finding that this code is really doing anything, other than creating confusion... I tried using Enter within the input field, but the calendar does not open... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm that's unfortunate. Without this code, there isn't really any way a screen reader can trigger the calendar. Not much of a difference since it didn't work anyways, but we should probably find a way to trigger the calendar for the simple variant, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The normal way to open stuff when navigating with a screen reader is in fact by using space, not enter - and that works in the simple variant when I focus at the label. But keyboard users will not be able to focus on this element.. I do not know what has been discussed about the simple variant, but maybe rather it should open automatically when receiving focus as there is no actual indication that the user should press either space or enter in the input field to open it. But I will leave it as is until now and this can be discussed separately 👍 |
||
} | ||
}; | ||
const hasTrigger = responsiveVariant === "with-trigger"; | ||
|
||
const onFieldClick = () => { | ||
if (!hasTrigger) { | ||
state.setOpen(true); | ||
} | ||
}; | ||
|
||
const hasTrigger = responsiveVariant === "with-trigger"; | ||
|
||
const styles = useMultiStyleConfig("Datepicker", {}); | ||
const popoverContent = ( | ||
<PopoverContent color="darkGrey" boxShadow="md" sx={styles.calendar}> | ||
<PopoverArrow sx={styles.arrow} /> | ||
<PopoverBody> | ||
<FocusLock> | ||
<Calendar | ||
{...calendarProps} | ||
showYearNavigation={showYearNavigation} | ||
/> | ||
</FocusLock> | ||
</PopoverBody> | ||
</PopoverContent> | ||
); | ||
|
||
return ( | ||
<I18nProvider locale={locale}> | ||
|
@@ -115,18 +124,14 @@ export const DatePicker = forwardRef<HTMLDivElement, DatePickerProps>( | |
<Popover | ||
{...dialogProps} | ||
isOpen={state.isOpen} | ||
onClose={state.close} | ||
onOpen={state.open} | ||
closeOnBlur | ||
closeOnEsc | ||
returnFocusOnClose | ||
onClose={state.close} | ||
> | ||
<InputGroup {...groupProps} display="inline-flex"> | ||
<PopoverAnchor> | ||
<StyledField | ||
variant={responsiveVariant} | ||
onClick={onFieldClick} | ||
onKeyPress={handleEnterClick} | ||
paddingX={3} | ||
minHeight={minHeight} | ||
> | ||
|
@@ -137,33 +142,24 @@ export const DatePicker = forwardRef<HTMLDivElement, DatePickerProps>( | |
label={props.label} | ||
labelProps={labelProps} | ||
name={props.name} | ||
ref={ref} | ||
ref={hasTrigger ? undefined : ref} | ||
{...fieldProps} | ||
/> | ||
</StyledField> | ||
</PopoverAnchor> | ||
{hasTrigger && <CalendarTriggerButton {...buttonProps} />} | ||
{hasTrigger && ( | ||
<PopoverTrigger> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙌 |
||
<CalendarTriggerButton ref={ref} {...buttonProps} /> | ||
</PopoverTrigger> | ||
)} | ||
</InputGroup> | ||
<FormErrorMessage {...errorMessageProps}> | ||
{errorMessage} | ||
</FormErrorMessage> | ||
{state.isOpen && !props.isDisabled && ( | ||
<Portal> | ||
<PopoverContent | ||
color="darkGrey" | ||
boxShadow="md" | ||
sx={styles.calendar} | ||
> | ||
<PopoverArrow sx={styles.arrow} /> | ||
<PopoverBody> | ||
<Calendar | ||
{...calendarProps} | ||
showYearNavigation={showYearNavigation} | ||
/> | ||
</PopoverBody> | ||
</PopoverContent> | ||
</Portal> | ||
{state.isOpen && !props.isDisabled && withPortal && ( | ||
<Portal>{popoverContent}</Portal> | ||
)} | ||
{state.isOpen && !props.isDisabled && !withPortal && popoverContent} | ||
</Popover> | ||
</Box> | ||
</I18nProvider> | ||
|
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.
Add a slightly better aria-label than just, e.g, "august 2023". The result will be "Calendar august 2023". The August 2023 part comes from the React Aria calendarProps