- Notifications
You must be signed in to change notification settings - Fork928
feat: form for editing ws schedule#1634
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Summary:This presents a form component and storybook. The UI will be a routedpage and added into the dashboard in a separate PR. It is likely aXService will be used at the page level to supply errors and actions tothis form.Impact of Change:Further progress on#1455Squashed Commits:* refactor: add className prop to Stackcombine classes with internal classes and an optional external classNameto better control the Stack.
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.
nice!
Uh oh!
There was an error while loading.Please reload this page.
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.
LGTM!
| control={<Checkbox checked={form.values.saturday} onChange={form.handleChange} name="saturday" />} | ||
| label={Language.daySaturdayLabel} | ||
| /> | ||
| </FormGroup> |
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.
Review: I can create amap for these as a refactor...will do in follow-up/later.
| initialValues: { | ||
| sunday: false, | ||
| monday: true, | ||
| tuesday: true, | ||
| wednesday: true, | ||
| thursday: true, | ||
| friday: true, | ||
| saturday: false, |
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.
(non-blocking): I'm not sure how this is going to be translated into a CRON string, but the below are all valid representations:
MON-FRIMON,TUE,THU,SUN1-32,3,4,5There 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 parent controlling the form will map thestartTime and each of these into an acceptable cron string.
It will likely do, format for simplicity. We will test each of these cases both with code and incrontab.guru.
However, this is all to say - that's a follow-up to the form, that doesn't need to know anything about cron. The code that maps between the form and API is dependency injected viaonSubmit, which allows us to test and maintain it in isolation, should something about our cron format change (which is nice).
* feat: ui for editing ws scheduleSummary:This presents a form component and storybook. The UI will be a routedpage and added into the dashboard in a separate PR. It is likely aXService will be used at the page level to supply errors and actions tothis form.Impact of Change:Further progress on#1455Squashed Commits:* refactor: add className prop to Stackcombine classes with internal classes and an optional external classNameto better control the Stack.* fix: getFormHelpers helperTextthe helperText logic was incorrect, the helperText would only show if not touched.
Summary:
This presents a form component and storybook. The UI will be a routed
page and added into the dashboard in a separate PR. It is likely a
XService will be used at the page level to supply errors and actions to
this form.
Impact of Change:
Further progress on#1455
Squashed Commits:
combine classes with internal classes and an optional external className
to better control the Stack.