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

refactor(date-selector): Refactor DateSelector component #313

Merged
merged 1 commit into from
May 21, 2024

Conversation

diogomateus
Copy link
Collaborator

@diogomateus diogomateus commented May 15, 2024

What this PR does

This PR refactors the date selector component to have 3 text inputs instead of 3 select inputs.
By setting 3 inputs it means that keyboard/typing navigation also needs some custom adjustments:

  • when reaching the end of input, focus changes to next input:
    ezgif-7-a14f96d45c

  • when using right/left arrows, it is possible to navigate to next/previous inputs (like tab and shit+tab). if there is a value already, input value is selected
    ezgif-7-37e49bad7e

  • backspace, if current focused input is empty, goes to previous input:
    ezgif-7-c31fadbe62

  • if day is 4 or more focus changes to month input, if month is 2 or more focus changes to year input:
    ezgif-7-85b713f993

How to test?

Test visiting the story:
http://localhost:9009/?path=/docs/jsx-dateselector--date-selector-story

Before:
Screenshot 2024-05-16 at 08 37 02

After:
Screenshot 2024-05-16 at 08 36 53

Checklist:

  • I reviewed my own code
  • The changes align with the designs I received
    Or give a reason why this does not apply:
  • I have attached screenshot(s), video(s) or gif(s) showing that the solution is working as expected
    Or give a reason why this does not apply:
  • I have updated the task(s) status on Linear
  • All new media is optimized (images, gifs, videos)

Browser support

My code works in the following browsers:

  • Firefox
  • Chrome
  • Safari
  • Edge

@diogomateus diogomateus force-pushed the diogo/date-selector branch 2 times, most recently from bed5aa0 to ef5a559 Compare May 15, 2024 16:18
@diogomateus diogomateus force-pushed the diogo/date-selector branch from ef5a559 to 8b5f1e7 Compare May 16, 2024 07:53
@diogomateus diogomateus marked this pull request as draft May 16, 2024 08:03
@diogomateus diogomateus force-pushed the diogo/date-selector branch from 8b5f1e7 to 9eb6abb Compare May 16, 2024 13:34
@diogomateus diogomateus marked this pull request as ready for review May 16, 2024 13:34
@busypeoples
Copy link
Contributor

busypeoples commented May 17, 2024

Two things I noticed: You could set a min of 1 and max of 12 for the month. Also if the year is invalid, you could highlight only the year as wrong so it is clear where the error is.

Another question: this will break all our end to end tests?

@diogomateus diogomateus force-pushed the diogo/date-selector branch from 9eb6abb to 106b0e3 Compare May 17, 2024 14:37
@diogomateus
Copy link
Collaborator Author

Two things I noticed: You could set a min of 1 and max of 12 for the month. Also if the year is invalid, you could highlight only the year as wrong so it is clear where the error is.

Another question: this will break all our end to end tests?

@busypeoples Added the year only validation.
And answering your questions:

  • For the min/max, I'm not using that because it needs input to be a number which would break some input selection features that needs to be added (you can view those on the above gif screenshots),
  • Regarding the e2e tests it will break but I've already updated them on a PR on app - it's mostly about replacing .select('May') with .type('5').

@diogomateus diogomateus merged commit 28009dc into main May 21, 2024
4 checks passed
@diogomateus diogomateus deleted the diogo/date-selector branch May 21, 2024 09:38
Copy link
Contributor

@talo242 talo242 left a comment

Choose a reason for hiding this comment

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

Works pretty well!

I'm requesting changes mostly to use the input labels instead of the data-testid.

dayjs(date).isAfter(dateCalendarFromMonth) ||
dayjs(date).isBefore(dateCalendarToMonth)
) {
const selectedDate = dayjs(date).format(dateFormat);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we are not checking if dateFormat is a valid date until the component performs an update.

Do you think we could fail earlier?
E.g. throw and exception during development if the dateFormat is incorrect.


await user.type(getByTestId('date-selector-day'), '5');
await user.type(getByTestId('date-selector-month'), '7');
await user.type(getByTestId('date-selector-year'), '2023');
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure we are not impacting accessibility, IMHO it'd be better if we stick to the actual input labels, and not rely on data-testid.

Is there a reason to use data-testid to target these inputs?

'data-testid': `date-selector-${key}`,
className: styles[`${key}Input`],
id: key,
name: key,
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we use this as uncontrolled component?

To give you more context, on the task engine we are adding a specific name and id to each input.
For example, a text input would have an id like this: input-f8556724-a703-455e-a6f5-16b0d16bb26d

But for the date input, we want the full date, so we are doing a tiny hack by using a hidden field with the full date.

I'm wondering if we should have such hidden field already inside the component instead of the individual name for each input.

Happy to discuss this a bit further. We don't need to change anything for this PR.

required: true,
label: placeholders?.[key],
placeholder: placeholders?.[`${key}Format` as FormatPlaceholder] ?? "",
labelInsideInput: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we not allowing this to be configured via props?

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

Successfully merging this pull request may close these issues.

4 participants