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

fix(components): Keep only one DatePicker open at a time #2397

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

taylorvnoj
Copy link
Contributor

@taylorvnoj taylorvnoj commented Feb 25, 2025

Motivations

If an InputDate is open and focus is shifted to another InputDate, they will both stay open. This can cause UX confusion & unintended side effects, especially when inputs are very close together. This PR addresses that and makes sure only 1 DatePicker can be open at a time.

Changes

Added

  • Added a Date Range story example (for testing purposes but also, because we should have this example as it's a common pattern)

Changed

  • Improved event propagation to parent components when DatePicker handles ESC key
  • We're now checking if there's already a picker open that's different from the current picker; only keeping 1 picker open at a time

Testing

  • test localhost Date Range story here
  • make sure only 1 picker remains open when clicking back and forth; also make sure clicking away from the picker and the ESC key still close the picker
  • in the Date Range story example, you can swap out one of the <InputDate /> for an <InputText /> and when focus goes to the InputText, the picker closes

Changes can be
tested via Pre-release


In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

Copy link

cloudflare-workers-and-pages bot commented Feb 25, 2025

Deploying atlantis with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8da1e03
Status: ✅  Deploy successful!
Preview URL: https://29366166.atlantis.pages.dev
Branch Preview URL: https://job-116104-job-116904-one-da.atlantis.pages.dev

View logs

@taylorvnoj taylorvnoj changed the title first pass at allowing one Datepicker to be open at a time fix(components): Keep only one DatePicker open at a time Feb 25, 2025
@taylorvnoj taylorvnoj self-assigned this Feb 25, 2025
@@ -115,7 +117,7 @@ export function DatePicker({
const datePickerClassNames = classnames(styles.datePicker, {
[styles.inline]: inline,
});
const { pickerRef } = useEscapeKeyToCloseDatePicker(open, ref);
const { pickerRef } = useEscapeKeyToCloseDatePicker(open);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ref removed from here wasn't needed for ESC key functionality; ESC key handler only needs pickerRef to call setOpen(false) and doesn't need the wrapper's ref.

Comment on lines +175 to +182
if (
openDatePickerRef.current &&
openDatePickerRef.current !== pickerRef.current
) {
openDatePickerRef.current.setOpen(false);
}

openDatePickerRef.current = pickerRef.current;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checks if there's already an open picker that's different from the current one and if found, closes the other picker.

Copy link

Published Pre-release for 8da1e03 with versions:

  - @jobber/[email protected]+8da1e03a

To install the new version(s) for Web run:

npm install @jobber/[email protected]+8da1e03a

@taylorvnoj taylorvnoj marked this pull request as ready for review February 26, 2025 02:22
@taylorvnoj taylorvnoj requested a review from a team as a code owner February 26, 2025 02:22
@Aiden-Brine Aiden-Brine self-requested a review February 26, 2025 16:54
Aiden-Brine
Aiden-Brine previously approved these changes Feb 26, 2025
Copy link
Contributor

@Aiden-Brine Aiden-Brine left a comment

Choose a reason for hiding this comment

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

Looks great, nice fix 👍

MichaelParadis
MichaelParadis previously approved these changes Feb 26, 2025
Copy link
Contributor

@MichaelParadis MichaelParadis left a comment

Choose a reason for hiding this comment

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

LGTM as well!

@taylorvnoj taylorvnoj dismissed MichaelParadis’s stale review February 26, 2025 21:48

As discussed with the team we are going to try a different approach; I'm currently using a global variable which is a React anti-pattern.

@jobbiebot jobbiebot bot removed the approved label Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants