Skip to content
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

Calendar and DatePicker audits and chromatic stories #3046

Merged
merged 19 commits into from
Apr 21, 2022
Merged

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Apr 19, 2022

Fixes #2751. Fixes #1682.

  • Updates keyboard paging behavior as discussed in Streamline the touch screen reader experience for Calendar #3032 (cc. @majornista)
  • Adds SSR tests and fixes failures.
  • Adds support for FocusableRef interface on all components.
  • Adds support for passing through DOM props like data attributes
  • Adds support for controlled isOpen props and shouldFlip to DatePicker
  • Adds chromatic stories for all components and fixes some styling issues found
  • API audit
  • Adds missing docs

export type {AriaDateFieldProps, DateFieldAria} from './useDateField';
export type {DatePickerAria} from './useDatePicker';
export type {DateRangePickerAria} from './useDateRangePicker';
export type {DateSegmentAria} from './useDateSegment';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New pattern? We aren't currently very consistent about what types we export from our packages. I was thinking we could try to export all input and output types from the hooks explicitly. Note that this also includes some re-exports from @react-types as it avoids users needing to install another package separately to use them.

title: 'Calendar',
parameters: {
chromaticProvider: {
locales: ['en-US'/* , 'ar-EG', 'ja-JP' */]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on translations

@@ -32,6 +32,11 @@ const OPEN_STATES = {
*/

export function OpenTransition(props) {
// Do not apply any transition if in chromatic.
if (process.env.CHROMATIC) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stripped by Parcel during the build so won't be published

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@majornista majornista self-requested a review April 19, 2022 16:58
majornista
majornista previously approved these changes Apr 19, 2022
Copy link
Collaborator

@majornista majornista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keyboard navigation seems to work as expected now. PageUp/PageDown always increments/decrements month, regardless of the visible range.

@majornista
Copy link
Collaborator

majornista commented Apr 19, 2022

Separate issue: this is definitely an edge case, perhaps only of interest to peculiar folk who want to figure out on what day of the week Christmas in 127 BCE would fall, but I encounter the error “Too many re-renders. React limits the number of renders to prevent an infinite loop,” when navigating moving between some eras using the Calendar. It happens in the Gregorian calendar, but it’s easier to test with the Japanese calendar and it seems to happen with all calendar systems.

  1. Open https://reactspectrum.blob.core.windows.net/reactspectrum/f056588fa1eb4fd124d5ec4aa0899ccee891[…]ex.html?path=/story/date-and-time-calendar--default and select the Japanese calendar from the second Picker.
  2. Hold down Shift+PageUp (fn + Shift + ArrowUp on a Mac laptop keyboard) to decrement by year until you reach April 2 Meiji.
  3. Press Shift+PageUp (fn + Shift + ArrowUp on a Mac laptop keyboard) to decrement to the last year of this first era of the modern Japanese calendar and React throws an error.

@devongovett
Copy link
Member Author

@majornista interesting find. The problem in that case is that the Japanese calendar implementation currently doesn't support dates before the Meiji era, which begins September 8, 1868 Gregorian. Therefore, April of that Gregorian year doesn't exist according to the implementation. Japanese eras get fuzzy before then, a lot of data is required to support them, and I didn't think it was terribly important for our current use cases to support.

For now, I've fixed the issue by constraining the date within supported eras. If you go back to September 1 Meiji, you'll see the month appears to start on day 8 and navigating to previous dates does nothing.

I didn't see the issue with the Gregorian calendar. How did you reproduce?

LFDanLu
LFDanLu previously approved these changes Apr 20, 2022
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, confirmed the new PageUp/Down/Home/End keyboard functionality + the Japanese date constraints. Still going through the chromatic stories but approving for now

@adobe-bot
Copy link

Build successful! 🎉

snowystinger
snowystinger previously approved these changes Apr 21, 2022
…cleanup

# Conflicts:
#	packages/@react-aria/calendar/src/types.ts
#	packages/@react-aria/calendar/src/useCalendarBase.ts
#	packages/@react-aria/calendar/src/useCalendarGrid.ts
#	packages/@react-aria/calendar/src/useRangeCalendar.ts
#	packages/@react-spectrum/calendar/src/CalendarBase.tsx
#	packages/@react-spectrum/calendar/src/CalendarMonth.tsx
#	packages/@react-spectrum/calendar/test/CalendarBase.test.js
@devongovett devongovett dismissed stale reviews from snowystinger and LFDanLu via 2b7bef1 April 21, 2022 16:32
@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine after merge :) gave it a quick run in chrome

@devongovett devongovett merged commit 3419dee into main Apr 21, 2022
@devongovett devongovett deleted the calendar-cleanup branch April 21, 2022 16:53
@majornista
Copy link
Collaborator

@majornista interesting find. The problem in that case is that the Japanese calendar implementation currently doesn't support dates before the Meiji era, which begins September 8, 1868 Gregorian. Therefore, April of that Gregorian year doesn't exist according to the implementation. Japanese eras get fuzzy before then, a lot of data is required to support them, and I didn't think it was terribly important for our current use cases to support.

For now, I've fixed the issue by constraining the date within supported eras. If you go back to September 1 Meiji, you'll see the month appears to start on day 8 and navigating to previous dates does nothing.

I didn't see the issue with the Gregorian calendar. How did you reproduce?

  1. Open https://codesandbox.io/s/fervent-mirzakhani-qkyzpi?file=/src/App.js
  2. Move keyboard focus to the selected date for the defaultValue in the calendar, January 1, 1 AD.
  3. Press ArrowUp to try navigating to the previous week or PageUp for the previous month.
  4. The error will be thrown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants