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

[@mantine/dates] TimeInput: Add format and clearable prop #607

Merged
merged 8 commits into from
Jan 5, 2022
Merged

[@mantine/dates] TimeInput: Add format and clearable prop #607

merged 8 commits into from
Jan 5, 2022

Conversation

jerebtw
Copy link
Contributor

@jerebtw jerebtw commented Jan 1, 2022

Fix for #581 and #580

This also can add a new hook (if wanted) called use-valid-state. (It's similar to useState but accepts a validation function and returns the last valid value, a set value function and the current value)

@jerebtw jerebtw changed the title Time input clean [@mantine/dates] TimeInput: Add format and clearable prop Jan 1, 2022
@jerebtw jerebtw requested a review from rtivital January 2, 2022 16:09
@rtivital
Copy link
Member

rtivital commented Jan 3, 2022

  • When AmPmInput is focused, there is no way to escape it with keyboard – neither Tab nor Shift + Tab works
  • Right/Left arrows should not change am/pm only up and down (to keep things consistent with other fields)
  • Please add a separate demo for clearable feature
  • Since we are introducing empty field handling, input should be empty if value is undefined
  • Current placeholder should be -- : -- : am to be more descriptive
  • If field is not filled text should be aligned to the left
  • When user enters hours, minutes and seconds should not be autofilled (see video with incorrect behavior)
2022-01-03.17.56.58.mov

@jerebtw
Copy link
Contributor Author

jerebtw commented Jan 3, 2022

Since we are introducing empty field handling, input should be empty if value is undefined

Should I add a prop to allow empty fields or this will be a breaking change because we won't have the new Date() as default Value

When user enters hours, minutes and seconds should not be autofilled (see video with incorrect behavior)

This is a problem with the current approach how we save the state because its currently a Date object and if we want your described function then we need to save them individual.

@rtivital
Copy link
Member

rtivital commented Jan 3, 2022

Should I add a prop to allow empty fields or this will be a breaking change because we won't have the new Date() as default Value

No, that's fine, it's not actually breaking

This is a problem with the current approach how we save the state because its currently a Date object and if we want your described function then we need to save them individual.

Well, I guess it's the only option then

@jerebtw
Copy link
Contributor Author

jerebtw commented Jan 3, 2022

@rtivital could you please look at this again, Thanks

Copy link
Member

@rtivital rtivital left a comment

Choose a reason for hiding this comment

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

It works fine, great job! There are several issues left to fix, and it can be merged.

docs/src/docs/dates/time-input.mdx Outdated Show resolved Hide resolved
docs/src/docs/hooks/use-validated-state.mdx Outdated Show resolved Hide resolved
docs/src/docs/hooks/use-validated-state.mdx Outdated Show resolved Hide resolved
docs/src/docs/hooks/use-validated-state.mdx Outdated Show resolved Hide resolved
src/mantine-dates/src/components/TimeInput/TimeInput.tsx Outdated Show resolved Hide resolved
src/mantine-dates/src/components/TimeInput/demos/usage.tsx Outdated Show resolved Hide resolved
@jerebtw jerebtw requested a review from rtivital January 4, 2022 16:17
@rtivital rtivital changed the base branch from dev to next-minor January 5, 2022 08:43
@rtivital rtivital merged commit 28f6050 into mantinedev:next-minor Jan 5, 2022
@rtivital
Copy link
Member

rtivital commented Jan 5, 2022

All good, thanks!

@jerebtw jerebtw deleted the TimeInputClean branch January 5, 2022 14:43
@brunovegreville
Copy link

Hey everyone, @rtivital @jerebtw !
Thanks for adding a 12-hour format to the TimeInput!
I just wanted to highlight that you might have broken support for non-qwerty keyboards by using event.nativeEvent.code === 'KeyP' and event.nativeEvent.code === 'KeyA' in src/mantine-dates/src/components/TimeInputBase/AmPmInput/AmPmInput.tsx (versus a classic e.key === "a")

On french keyboards for instance, typing "am" does not change the AmPm input value, but typing qm does (because q maps to KeyA)

Let me know if you can do something about it 🙏
Thanks!

@rtivital
Copy link
Member

rtivital commented Jan 23, 2022

@brunovegreville do people in France use am/pm time format?

@jerebtw
Copy link
Contributor Author

jerebtw commented Jan 23, 2022

Thanks for pointing this out i will look in to it

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.

3 participants