-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
feat(ui): Display pretty cron schedule #5088
Conversation
alexec
commented
Feb 11, 2021
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Ah, I though you had meant a cron scheduler builder. Not one to simply display a humanized version :) |
A builder could be a v2. We've both looked for libraries, and not found a good builder library. |
@simster7 could I please request your review? |
<div className='columns small-2'>{w.spec.schedule}</div> | ||
<div className='columns small-2'> | ||
<div className='columns small-3'> | ||
<Schedule schedule={w.spec.schedule} /> |
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.
I don't think the "pretty" schedule should be displayed in the list view. It's not necessary when looking at all cron workflows (as opposed to just one in detail) and from your screenshot it looks like they're frequently cut off (bad UX)
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.
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.
I can see a bug in the library. 0 12 * */1 *
. This should be "At 12:00 in every month.”
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.
huh - 1st of the month?
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.
I think "/1" === ""?
@@ -12,6 +13,7 @@ export const CronWorkflowSpecEditor = ({onChange, spec}: {spec: CronWorkflowSpec | |||
<div className='columns small-3'>Schedule</div> | |||
<div className='columns small-9'> | |||
<TextInput value={spec.schedule} onChange={schedule => onChange({...spec, schedule})} /> | |||
<ScheduleValidator schedule={spec.schedule} /> |
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.
I think this should be inline with the input, not in a new line. The spec string will never be long enough to occupy even a tenth of the length of the input box, so no reason we can't use the remaining length more effectively with the validator
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.
I'd like to leave this, it is a small panel and I don't think it should be optimised.
Co-authored-by: Simon Behar <simbeh7@gmail.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
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.
Pre approval
Signed-off-by: Alex Collins <alex_collins@intuit.com>
I've pork-barrelled changes to formatting durations. We can rollback if disliked |