-
Notifications
You must be signed in to change notification settings - Fork 95
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
Fixes #38059 - job wizard limit starts at to future only #932
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the few things I commented on, it works great
@@ -432,7 +450,8 @@ export const JobWizard = ({ rerunData }) => { | |||
valid.advanced && | |||
valid.schedule && | |||
!isSubmitting && | |||
!isStartsBeforeError, | |||
!isStartsBeforeError && | |||
!isStartsAtError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Next" button is enabled even when the isStartsAtError
occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added restrictions to the schedule steps
<Alert | ||
ouiaId="starts-at-error-alert" | ||
variant="danger" | ||
title={__("'Starts at' date must in the future")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing word: "...date must be in the..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed for both errors
@@ -16,3 +16,18 @@ export const StartsBeforeErrorAlert = () => ( | |||
<Divider component="div" /> | |||
</> | |||
); | |||
|
|||
export const StartsAtErrorAlert = () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the file could be renamed so it's not confusing when it includes StartsAtErrorAlert
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to StartsErrorAlert
c665348
to
e9f88d1
Compare
e9f88d1
to
57186da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, it can be merged - @adamruzicka can you please do it?
Limited most of the date pickers to be only in the future (expect the user input one).
Added an error shown when the "starts at" is in the past.
Set the default value to starts at one minute on the future.
Updated the useEffect to set errors on "future" and "recurring" schedule, and to remove them otherwise.
Since I added a timer in the useEffect I added test fixes/ adjustments.