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

feat(DatePicker): Migrate to react-day-picker v8 #3001

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jpveooys
Copy link
Contributor

@jpveooys jpveooys commented Jan 18, 2022

Related issue

#2871
#2872

Overview

Work in progress PR to migrate DatePicker to react-day-picker v8.

Link to preview

todo

Reason

To resolve various accessibility and keyboard navigation problems.

Work carried out

  • Migrate DatePicker to react-day-picker v8
  • Fix bug with shading of keyboard range selection when second date is blurred
  • Focus selected date or current date on open

Demo

todo

Developer notes

todo

@jpveooys jpveooys added Status: On hold Issue progression is temporarily paused Package: react-component-library Package/code type Type: Development Related to a design system component labels Jan 18, 2022
@jpveooys jpveooys self-assigned this Jan 18, 2022
@jpveooys jpveooys force-pushed the feature/date-picker-e-v8 branch 5 times, most recently from b7a36d7 to 14c90da Compare January 24, 2022 16:59
@sonarcloud
Copy link

sonarcloud bot commented Jan 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

88.5% 88.5% Coverage
0.0% 0.0% Duplication

@github-actions github-actions bot added the Stale label Apr 18, 2022
@jpveooys jpveooys force-pushed the feature/date-picker-e-v8 branch 2 times, most recently from 4f11222 to 2ab8f7e Compare May 5, 2022 16:00
@jpveooys jpveooys changed the title feat(DatePickerE): Migrate to react-day-picker v8 feat(DatePicker): Migrate to react-day-picker v8 May 5, 2022
@jpveooys jpveooys force-pushed the feature/date-picker-e-v8 branch 3 times, most recently from 374beb0 to 8f34471 Compare May 6, 2022 09:24
@github-actions github-actions bot removed the Stale label Aug 1, 2022
This migrates DatePicker to version 8 of react-day-picker, which includes various important accessibility and keyboard navigation fixes.
This fixes a small bug where when a second date of a range was being selected using the keyboard, the range would stay shading after tabbing from the second date to a different element.
This makes focusing consistent with the W3 WAI-ARIA date picker example.
Copy link

@c9qsm62x c9qsm62x left a comment

Choose a reason for hiding this comment

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

https://github.com/defencedigital/mod-uk-design-system/blob/9eb0eb2597b4cbcba88d0e1c2a158ec0e3a9c3ba/packages/react-component-library/src/components/DatePicker/DatePicker.test.tsx#L962-L972

We have failing test here but was wondering if this is needed because I cant thing of a scenario when this will happen as its readOnly

@jpveooys
Copy link
Contributor Author

@c9qsm62x It was added for coverage IIRC as those code paths still exist in the useInput hook.

I think the test has stopped doing what it was meant to since user-event was updated to v14 – fireEvent.keyDown(input, { key: 'Enter' }) probably makes more sense.

@c9qsm62x
Copy link

Test
DatePicker › when a single date picker is rendered and the picker is opened › when Shift-Tab is pressed once › traps the focus within the picker

it looks like this behaviour has changed in V8
https://react-day-picker.js.org/basics/keyboard#example
Having a look at this example it looks like when a date is in focus and you press shift+tab it will focus on the next month button.

@jpveooys
Copy link
Contributor Author

Yep that test looks wrong now as the initial focus is now set to the selected date.

Should be something like this I think:

    describe('when Tab is pressed once', () => {
      beforeEach(async () => {
        await user.tab()
      })

      it('traps the focus within the picker', () => {
        expect(
          wrapper.getByLabelText(PREVIOUS_MONTH_BUTTON_LABEL)
        ).toHaveFocus()
      })

      describe('and Shift-Tab is then pressed once', () => {
        beforeEach(() => {
          return user.tab({ shift: true })
        })

        it('still traps the focus within the picker', () => {
          expect(
            wrapper.getByRole('button', { name: '5th December (Thursday)' })
          ).toHaveFocus()
        })
      })
    })

@github-actions github-actions bot added the Stale label Oct 17, 2022
@m7kvqbe1
Copy link
Member

Closing for now because it's very stale - hoping to come back to this soon.

@m7kvqbe1 m7kvqbe1 closed this Apr 30, 2024
@m7kvqbe1 m7kvqbe1 reopened this Jun 19, 2024
@m7kvqbe1 m7kvqbe1 assigned m7kvqbe1 and unassigned jpveooys Jun 19, 2024
@github-actions github-actions bot removed the Stale label Jun 24, 2024
@github-actions github-actions bot added the Stale label Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: react-component-library Package/code type Stale Status: On hold Issue progression is temporarily paused Type: Development Related to a design system component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants