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

Adds timezone support to DatePicker #2293

Merged
merged 15 commits into from
Jul 31, 2024

Conversation

bryancunningham-okta
Copy link
Contributor

OKTA-729288

Summary

Adds time zone support by

  • allowing timeZone, timezoneOptions, timeZoneLabel props.
  • renders TimeZonePicker if timezoneOptions are provided.

Testing & Screenshots

  • I have confirmed this change with my designer and the Odyssey Design Team.

@bryancunningham-okta bryancunningham-okta requested a review from a team as a code owner July 18, 2024 15:08
Comment on lines +82 to +88
export const useOdysseyDateFields = ({
defaultValue,
isDateEnabled = () => true,
isMonthEnabled = () => true,
isYearEnabled = () => true,
minDate: minDateProp,
maxDate: maxDateProp,
Copy link
Contributor

Choose a reason for hiding this comment

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

I skimmed this file. I'm gonna assume it's good.

@bryancunningham-okta bryancunningham-okta force-pushed the bc_OKTA-729288-datepicker-add-timezome branch from e513073 to 56b2c3a Compare July 26, 2024 17:34
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

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

Code is fine. We've reviewed it, and it's good!

I'm concerned about the 5-second time before the error displays. I'd like to only have it on Blur or talk to other design folks and see what they think. We don't have this anywhere else (displaying an error message ourselves), so I'd like to get confirmation.

We should also look at potentially adding more validation functionality to Odyssey if we want it to function a certain way.

(validationError) => {
const timeoutId = setTimeout(() => {
setDisplayedErrorMessage(validationError);
}, 5000);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put this 5000 as a constant at the top of the file.

@bryancunningham-okta bryancunningham-okta force-pushed the bc_OKTA-729288-datepicker-add-timezome branch from 05bb779 to b03118b Compare July 31, 2024 17:29
@oktapp-aperture-okta oktapp-aperture-okta bot merged commit 7ba8999 into main Jul 31, 2024
1 of 2 checks passed
@oktapp-aperture-okta oktapp-aperture-okta bot deleted the bc_OKTA-729288-datepicker-add-timezome branch July 31, 2024 18:02
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.

2 participants