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(DatePickerE): Initial updates based on v3 design spec #2841

Merged
merged 11 commits into from
Dec 7, 2021

Conversation

jpveooys
Copy link
Contributor

@jpveooys jpveooys commented Nov 26, 2021

Related issue

Resolves #2439

Overview

This makes various updates to the DatePickerE component based on the v3 design spec.

Link to preview

Reason

Incremental work towards the v3 release.

Work carried out

  • Update the styling of the input control
  • Update the styling of the day picker
  • Update the styling and positioning of the floating box
  • Remove isValid prop
  • Append date format to label automatically
  • Reuse partials from TextInputE
  • Add hover shading for intermediate dates in range mode
  • Update placeholder and automatic opening behaviour

Demo

Single date

date-picker-e-single

Range

date-picker-e-range

Developer notes

Does not include:

@jpveooys jpveooys self-assigned this Nov 29, 2021
@jpveooys jpveooys added Package: react-component-library Package/code type Type: Development Related to a design system component labels Nov 29, 2021
@jpveooys jpveooys force-pushed the feature/date-picker-v3-pt-2 branch 6 times, most recently from d9777c7 to d848307 Compare November 30, 2021 17:03
@jpveooys jpveooys changed the title Update DatePickerE behaviour and styling feat(DatePickerE): Initial updates based on v3 design spec Nov 30, 2021
@jpveooys jpveooys force-pushed the feature/date-picker-v3-pt-2 branch 2 times, most recently from 020125c to 8a62cc5 Compare December 1, 2021 14:50
@@ -153,20 +161,20 @@ export const DatePickerE: React.FC<DatePickerEProps> = ({
>
<StyledOuterWrapper
data-testid="datepicker-outer-wrapper"
$hasFocus={open}
$hasFocus={hasFocus && !hasError}
Copy link
Contributor Author

@jpveooys jpveooys Dec 1, 2021

Choose a reason for hiding this comment

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

The && !hasError is temporary here to preserve the existing behaviour (which is covered by tests). It will be reviewed as part of #2863.


const { color } = selectors

export const StyledFloatingBox = styled(FloatingBox)`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left these as overrides for now as it's not yet clear if these will be directly applicable anywhere else.

@jpveooys jpveooys marked this pull request as ready for review December 1, 2021 15:51
@jpveooys
Copy link
Contributor Author

jpveooys commented Dec 2, 2021

@k9dh3zij I've added the outer shadow to the calendar sheet. Please have a look if it's as intended, as how it looks will depend on the origin (in relation to the other borders and shadows).

@k9dh3zij
Copy link

k9dh3zij commented Dec 2, 2021

Thanks @jpveooys loving this! Feedback:

Visual

  • The button looks slightly squashed - should be consistent with other components:
    image

  • When filled, the input label should be 2px higher and the user input should be 1px higher. Please note: I think this needs to change across all experimental inputs - not sure if I missed it in previous PRs or if something changed, sorry
    Visual comparison with design:
    image

  • As already discussed, missing Sheet drop shadow

Keyboard controls

  • When opening the Sheet, can we move focus to the currently selected date? (as in https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/datepicker-dialog.html)

  • When closing the Sheet, can we return focus to the button? (as in the W3 example)

  • Range date picker: when using the keyboard to select the end-date, the visual highlight between start date and currently focused end date doesn't work
    image

  • Can we make it so that the Sheet contains its own tab sequence. (Tab and Shift + Tab do not move focus outside the dialog)

  • Further to the above, can we add Esc as a shortcut to close the Sheet please?

That's all for now, I think 😅 Thanks!

disabled={isDisabled}
data-testid="datepicker-input-button"
>
<StyledSeparator />
<DropdownIndicatorIcon isOpen={open} />
<StyledIconCalendarWrapper>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noticed StyledIconCalendarWrapper is misnamed – will rename to StyledIconEventWrapper.

@jpveooys
Copy link
Contributor Author

jpveooys commented Dec 6, 2021

@k9dh3zij

I've made updates to address those except for the following two:

When opening the Sheet, can we move focus to the currently selected date? (as in https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/datepicker-dialog.html)

There was an issue already open in the underlying library we're using (react-day-picker) regarding this. I've created a separate issue #2871 at our end with more info and responded to the issue in the underlying library.

Can we make it so that the Sheet contains its own tab sequence. (Tab and Shift + Tab do not move focus outside the dialog)

I found a library that will add this behaviour (called a 'focus trap'). However, it has a bug where the tab key stops functioning when dates after the first of the month are focused, so I've left that change out and created a separate issue (#2870). I also need to report the bug upstream.

Regarding:

Range date picker: when using the keyboard to select the end-date, the visual highlight between start date and currently focused end date doesn't work

I've added this but note it's not possible at the moment to clear that highlight when tabbing away with react-day-picker v7 (it will be with v8 which is currently in beta). (Have created a separate issue for this as well: #2872.)

I also updated the border radius of the sheet and the hover/pressed/focus styles of the next and previous month buttons, as they seemed wrong before to me. Have another look at those if you can 🙂

Copy link
Collaborator

@thyhjwb6 thyhjwb6 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@@ -605,6 +605,32 @@ describe('DatePickerE', () => {
expect(wrapper.getByText('December 2019')).toBeInTheDocument()
})

describe.each([
Copy link
Collaborator

Choose a reason for hiding this comment

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

👌

@m7kvqbe1
Copy link
Member

m7kvqbe1 commented Dec 6, 2021

Are the disabled state styles correct (currently the picker button is blue and clickable):

Screenshot 2021-12-06 at 14 38 39

Copy link
Member

@m7kvqbe1 m7kvqbe1 left a comment

Choose a reason for hiding this comment

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

LGTM - just a couple of tiny comments

@jpveooys
Copy link
Contributor Author

jpveooys commented Dec 6, 2021

Are the disabled state styles correct (currently the picker button is blue and clickable):

Good spot! Have updated those.

Copy link
Member

@m7kvqbe1 m7kvqbe1 left a comment

Choose a reason for hiding this comment

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

👍

@k9dh3zij
Copy link

k9dh3zij commented Dec 7, 2021

Thanks for the updates @jpveooys looking great!

As discussed, something that we can look to improve when the react-day-picker library allows it, please:

  • Disabled dates shouldn't get focus. Therefore, the state shown here should not be possible:
    image

Big 👍 from me on this! :)

@jpveooys
Copy link
Contributor Author

jpveooys commented Dec 7, 2021

Thanks @k9dh3zij !

It looks like that's covered by gpbl/react-day-picker#1320 which was coincidentally opened 30 minutes ago 😄

Update general styling based on the v3 design spec.

Includes removing the `isValid` prop.
Make the placeholder big when the input is empty, and don't automatically open the picker when the input is focused.

Also append the date format to the label automatically.
Update the styling of the floating box using overrides.

(Currently not clear if these styles are applicable for anything else, therefore they are left as overrides for now.)
Add shading of intermediate days on hover, and use a consistent order for the selection of the start and end dates.
@sonarcloud
Copy link

sonarcloud bot commented Dec 7, 2021

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 6 Code Smells

91.2% 91.2% Coverage
0.0% 0.0% Duplication

@jpveooys jpveooys merged commit 7802f83 into master Dec 7, 2021
@jpveooys jpveooys deleted the feature/date-picker-v3-pt-2 branch December 7, 2021 14:04
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 Type: Development Related to a design system component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial iteration of DatePickerE
4 participants