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

Cut 965 date time input default time #2725

Merged
merged 15 commits into from Feb 22, 2024
Merged

Conversation

leventekobor
Copy link
Contributor

Summary

Adds the defaultDaySelectionTime prop to DateTimeInput component, and exposes it in the DateTimeField component

@leventekobor leventekobor added the 🙏 Status: Dev Review Waiting for technical reviews label Feb 19, 2024
@leventekobor leventekobor self-assigned this Feb 19, 2024
@leventekobor leventekobor requested a review from a team as a code owner February 19, 2024 07:48
Copy link

changeset-bot bot commented Feb 19, 2024

🦋 Changeset detected

Latest commit: 456b6ab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 95 packages
Name Type
@commercetools-uikit/date-time-field Minor
@commercetools-uikit/date-time-input Minor
@commercetools-uikit/calendar-time-utils Minor
@commercetools-uikit/fields Minor
@commercetools-uikit/inputs Minor
@commercetools-uikit/date-input Minor
@commercetools-uikit/date-range-input Minor
@commercetools-frontend/ui-kit Minor
@commercetools-uikit/date-field Minor
@commercetools-uikit/date-range-field Minor
@commercetools-uikit/design-system Minor
@commercetools-uikit/calendar-utils Minor
@commercetools-uikit/hooks Minor
@commercetools-uikit/i18n Minor
@commercetools-uikit/localized-utils Minor
@commercetools-uikit/utils Minor
@commercetools-uikit/accessible-hidden Minor
@commercetools-uikit/avatar Minor
@commercetools-uikit/card Minor
@commercetools-uikit/collapsible-motion Minor
@commercetools-uikit/collapsible-panel Minor
@commercetools-uikit/collapsible Minor
@commercetools-uikit/constraints Minor
@commercetools-uikit/data-table-manager Minor
@commercetools-uikit/data-table Minor
@commercetools-uikit/field-errors Minor
@commercetools-uikit/field-label Minor
@commercetools-uikit/field-warnings Minor
@commercetools-uikit/grid Minor
@commercetools-uikit/icons Minor
@commercetools-uikit/label Minor
@commercetools-uikit/link Minor
@commercetools-uikit/loading-spinner Minor
@commercetools-uikit/messages Minor
@commercetools-uikit/notifications Minor
@commercetools-uikit/pagination Minor
@commercetools-uikit/primary-action-dropdown Minor
@commercetools-uikit/progress-bar Minor
@commercetools-uikit/stamp Minor
@commercetools-uikit/tag Minor
@commercetools-uikit/text Minor
@commercetools-uikit/tooltip Minor
@commercetools-uikit/view-switcher Minor
@commercetools-uikit/accessible-button Minor
@commercetools-uikit/flat-button Minor
@commercetools-uikit/icon-button Minor
@commercetools-uikit/link-button Minor
@commercetools-uikit/primary-button Minor
@commercetools-uikit/secondary-button Minor
@commercetools-uikit/secondary-icon-button Minor
@commercetools-uikit/async-creatable-select-field Minor
@commercetools-uikit/async-select-field Minor
@commercetools-uikit/creatable-select-field Minor
@commercetools-uikit/localized-multiline-text-field Minor
@commercetools-uikit/localized-text-field Minor
@commercetools-uikit/money-field Minor
@commercetools-uikit/multiline-text-field Minor
@commercetools-uikit/number-field Minor
@commercetools-uikit/password-field Minor
@commercetools-uikit/radio-field Minor
@commercetools-uikit/search-select-field Minor
@commercetools-uikit/select-field Minor
@commercetools-uikit/text-field Minor
@commercetools-uikit/time-field Minor
@commercetools-uikit/async-creatable-select-input Minor
@commercetools-uikit/async-select-input Minor
@commercetools-uikit/checkbox-input Minor
@commercetools-uikit/creatable-select-input Minor
@commercetools-uikit/input-utils Minor
@commercetools-uikit/localized-money-input Minor
@commercetools-uikit/localized-multiline-text-input Minor
@commercetools-uikit/localized-rich-text-input Minor
@commercetools-uikit/localized-text-input Minor
@commercetools-uikit/money-input Minor
@commercetools-uikit/multiline-text-input Minor
@commercetools-uikit/number-input Minor
@commercetools-uikit/password-input Minor
@commercetools-uikit/radio-input Minor
@commercetools-uikit/rich-text-input Minor
@commercetools-uikit/rich-text-utils Minor
@commercetools-uikit/search-select-input Minor
@commercetools-uikit/search-text-input Minor
@commercetools-uikit/select-input Minor
@commercetools-uikit/select-utils Minor
@commercetools-uikit/selectable-search-input Minor
@commercetools-uikit/text-input Minor
@commercetools-uikit/time-input Minor
@commercetools-uikit/toggle-input Minor
@commercetools-uikit/spacings-inline Minor
@commercetools-uikit/spacings-inset-squish Minor
@commercetools-uikit/spacings-inset Minor
@commercetools-uikit/spacings-stack Minor
@commercetools-uikit/buttons Minor
@commercetools-uikit/spacings Minor
visual-testing-app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Feb 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 22, 2024 8:57am

Copy link
Contributor

@kark kark left a comment

Choose a reason for hiding this comment

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

Thanks for working on this 🙌 Some feedback from my end.

packages/calendar-time-utils/src/calendar-time.ts Outdated Show resolved Hide resolved
@CarlosCortizasCT
Copy link
Contributor

I think we're still missing a couple of technical requirements:

  • The value must be validated and, if it’s not valid, we should log a warning to the console and use the default value.
  • Bear in mind that this (provided default time) is an internationalized value, so we need to transform the received value to the appropriate format based on the user’s locale (24 hours or AM/PM)

@leventekobor
Copy link
Contributor Author

hey @CarlosCortizasCT thank you for the feedback!

I think we're still missing a couple of technical requirements:

  • The value must be validated and, if it’s not valid, we should log a warning to the console and use the default value.

currently in calendar-time.ts (formatDefaultTime function) there is the following validation for the time: if (moment(time, 'HH:mm', true).isValid()) if this is false we have a warning in the console notifying the user. Is this type of validation is insufficient?

  • Bear in mind that this (provided default time) is an internationalized value, so we need to transform the received value to the appropriate format based on the user’s locale (24 hours or AM/PM)

for this also in calendar-time.ts there is the following: moment(today).locale(locale).format('LT'). should i add something else here as well?

@CarlosCortizasCT
Copy link
Contributor

hey @CarlosCortizasCT thank you for the feedback!

I think we're still missing a couple of technical requirements:

  • The value must be validated and, if it’s not valid, we should log a warning to the console and use the default value.

currently in calendar-time.ts (formatDefaultTime function) there is the following validation for the time: if (moment(time, 'HH:mm', true).isValid()) if this is false we have a warning in the console notifying the user. Is this type of validation is insufficient?

  • Bear in mind that this (provided default time) is an internationalized value, so we need to transform the received value to the appropriate format based on the user’s locale (24 hours or AM/PM)

for this also in calendar-time.ts there is the following: moment(today).locale(locale).format('LT'). should i add something else here as well?

I'm sorry Lucas, you're right.

Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT 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 to me now 💯

Thanks a lot for your collaboration, Levente 🙇

@leventekobor
Copy link
Contributor Author

Looks good to me now 💯

Thanks a lot for your collaboration, Levente 🙇

Thank you for the feedback and the help! are there any more steps? Should i tag anyone else in this PR?

Copy link
Contributor

@kark kark left a comment

Choose a reason for hiding this comment

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

Many thanks, looks good from my end as well.
I'm just leaving some final remarks.

Copy link
Contributor

@kark kark left a comment

Choose a reason for hiding this comment

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

Thank you 🙌

@kark kark merged commit 75fcd4f into main Feb 22, 2024
6 checks passed
@kark kark deleted the CUT-965-date-time-input-default-time branch February 22, 2024 10:30
@ct-changesets ct-changesets bot mentioned this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙏 Status: Dev Review Waiting for technical reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants