Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: edit workspace schedule page#1701

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

Merged
greyscaled merged 29 commits intomainfromvapurrmaid/gh-1455/part-3/page
May 26, 2022

Conversation

@greyscaled
Copy link
Contributor

@greyscaledgreyscaled commentedMay 24, 2022•
edited
Loading

Resolves:#1455
Resolves:#1456

Summary

Adds a page (accessible from Workspace Schedule section on a workspace) to edit a schedule.

State Machine

WorkspaceSchedule State Machine

Impact of Change

General parity with CLI for autostart/autostop: that is you can update your schedule from the UI

Demo of Change

edit-schedule

@greyscaledgreyscaled requested a review fromjohnstcnMay 24, 2022 06:05
Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

Looking good so far! Comments below.

greyscaled reacted with thumbs up emoji
Copy link
Member

@kylecarbskylecarbs left a comment

Choose a reason for hiding this comment

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

Code looks good to me, but skipping a thorough review until in a ready state 😄!

What do you think about Schedule being an option in the Settings pages? It could be helpful to the user for all editable options to be in one spot!

greyscaled reacted with thumbs up emoji
@greyscaled
Copy link
ContributorAuthor

What do you think about Schedule being an option in the Settings pages? It could be helpful to the user for all editable options to be in one spot!

In the short term, I don't want to do that for the reasons of:

  • we don't yet have the settings page or a design or really any thoughts of how it will look
  • the schedule form is complex and is represented as a full-page form for that reason right now

I do think it's the right call in the long(er) term, longer here is relative - could even be as early as next sprint (week). I don't want to pause getting it in here as our current workspace design has a sidebar that links out to the schedule page, and it fits in really nicely.

If we decide we have cycles to design the settings page, I'd absolutely believe this could be a candidate.

johnstcn and kylecarbs reacted with thumbs up emoji

@kylecarbs
Copy link
Member

Makes complete sense to me! Appreciate hearing the thought process 💡

@ammario
Copy link
Member

@vapurrmaid could you share what it looks like please?

@johnstcn
Copy link
Member

@ammario you can check theStorybook UI review

@greyscaledgreyscaled self-assigned thisMay 24, 2022
@greyscaledgreyscaled mentioned this pull requestMay 24, 2022
2 tasks
@ammario
Copy link
Member

ammario commentedMay 25, 2022•
edited
Loading

@johnstcn thanks for sharing the Storybook.

  1. Why did we omit the timezone field?
  2. Why was "runtime" selected instead of "TTL"?
  3. Why is runtime in minutes instead of hours?

@greyscaled
Copy link
ContributorAuthor

@johnstcn thanks for sharing the Storybook.

  1. Why did we omit the timezone field?
  2. Why was "runtime" selected instead of "TTL"?
  3. Why is runtime in minutes instead of hours?

There are no good reasons other than the initial draft, followed by iteration.

I'll change from runtime to TTL.

I'll change from minutes to hours.

I'll add a timezone field

@ammario
Copy link
Member

Sounds good@vapurrmaid. I think the UI is clean and professional and I'm excited to give it a swing!

Copy link
ContributorAuthor

@greyscaledgreyscaled left a comment•
edited
Loading

Choose a reason for hiding this comment

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

This PR is unfortunately large (though a lot of the code is test code, thankfully).

I am addingREVIEW comments to most diffs to add further explanation/context.

I'm happy to do a synchronous review in Discord if it's warranted.

Sorry that this one got large, I like to find smaller pieces, but this is just how things are as CE deadline approaches. Many thanks in advance 💖 .

Edit: Also, if you're newer to cron strings,https://crontab.guru/#30_09_*_*_1-5 is the way to go

Edit: If you're wondering why we don't use a cron library for some of our manipulation, it's because we have a custom implementation right now and do not want to expand it to full blast just yet.

Edit: This PR grew way larger than I had intended. I'm happy to address all comments, but my default response to things like refactors, improvements, nice-to-haves will be "iteration", as in follow-up efforts.

@johnstcn and I already have various follow-up efforts, like changing thettl from nanosecond precision. Some of the messy hours to nanosecond time conversion will get cleaned up then.

See also@ammario and I's awesome comment thread and a consensus here:#1701 (comment) that we want to give the user a diff-like preview in this form while editing it. That may not land in CE (I'll certainly try), as there are other priorities like notifications and extension that must come first.

<WorkspaceSchedulePage/>
</RequireAuth>
}
/>
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

A route/workspaces/:workspace/schedule was added to view the form

<LinkclassName={styles.scheduleAction}>{Language.editScheduleLink}</Link>
<LinkclassName={styles.scheduleAction}component={RouterLink}to={`/workspaces/${workspace.id}/schedule`}>
{Language.editScheduleLink}
</Link>
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review: The dashboard link to edit a workspace schedule now works!

Kira-Pilot and kylecarbs reacted with hooray emoji
action("onSubmit")
returnPromise.resolve()
},
onSubmit:()=>action("onSubmit"),
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

You'll see this show up in theWorkspaceScheduleForm component as well: I un-promisifiedonSubmit. I did this because the model goes to a new state once the promises resolve and the API calls are success. We use that state to navigate away from the page.

In other words, there is a bit of a 'fire-n-forget' from Formik's point of view, but since we also pass inisLoading to the form, which stems from the model, it still works out OK.

{...formHelpers("ttl",Language.ttlHelperText)}
inputProps={{min:0,step:30}}
disabled={form.isSubmitting||isLoading}
inputProps={{min:0,step:1}}
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

We changed from minutes to hours

{value:form.values.thursday,name:"thursday",label:Language.dayThursdayLabel},
{value:form.values.friday,name:"friday",label:Language.dayFridayLabel},
{value:form.values.saturday,name:"saturday",label:Language.daySaturdayLabel},
]
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

I refactored to use amap

Kira-Pilot reacted with thumbs up emoji
Comment on lines +94 to +107
startTime:Yup.string()
.ensure()
.test("is-time-string",Language.errorTime,(value)=>{
if(value===""){
returntrue
}elseif(!/^[0-9][0-9]:[0-9][0-9]$/.test(value)){
returnfalse
}else{
constparts=value.split(":")
constHH=Number(parts[0])
constmm=Number(parts[1])
returnHH>=0&&HH<=23&&mm>=0&&mm<=59
}
}),
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

We had no tests or validation onstartTime previously. See related tests inWorkspaceScheduleForm.test.tsx.

saturday:boolean

startTime:string
timezone:string
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

The form now has a newtimezone input, with validation and tests.

Kira-Pilot reacted with hooray emoji
Comment on lines +1 to +4
/**
*@fileoverview workspaceSchedule is an xstate machine backing a form to CRUD
* an individual workspace's schedule.
*/
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

Here's a GIF of me stepping through the machine states/possibilities. Please LMK if there are any questions.

To attempt and get ahead, I added a final statesubmitSuccess that's essentially a dupe of theidle state, but the page is using it to know tonavigate.

edit-schedule-model

Kira-Pilot and johnstcn reacted with heart emoji
Copy link
Member

Choose a reason for hiding this comment

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

This was so helpful; thank you!

* The 0th index is Sunday
* The 6th index is Saturday
*/
exporttypeWeeklyFlag=[boolean,boolean,boolean,boolean,boolean,boolean,boolean]
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

This type is very helpful and needed because we get some static analysis on the size of the arrays. The semantic meaning is like a bit mask where in the array, each index corresponds to a day and whether that day is "set" (true) or unset "false".

So in other words, it goes from Sunday -> Saturday.

This sort of thing is easier to understand than bit masks, and maps very cleanly to setting values for the checkbox group.

Kira-Pilot reacted with eyes emoji
}

returnresult
}
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

This is a "private" method (only meant to be used indowToWeeklyFlag and thus is not exported.

I'd review this before I'd reviewdowToWeeklyFlag.

The algorithm does the following:

  • takes a number (ex: 1)
  • or number range (ex: 1-5)

and returns the associatedWeeklyFlag array (see:https://github.com/coder/coder/pull/1701/files#r882360151).

I have a method that focuses just on number or range, because in the parent functiondowToWeeklyFlag, you can have comma-separated values like this:

1,3,5
1-2,4-5

So this function is called in a loop on each comma-separated group.

}

returnresults
}
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Review:

Before reading this function, I'd readhttps://github.com/coder/coder/pull/1701/files#r882361592

This builds uponprocessRangeOrNum by calling it in a loop after splitting the dow cron string by comma.

Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

Looking good!

Co-authored-by: Kira Pilot <kira@coder.com>
Copy link
Member

@kylecarbskylecarbs left a comment

Choose a reason for hiding this comment

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

Very thorough comments and tests!

@Kira-Pilot
Copy link
Member

I noticed that I can blissfully keyboard shortcut my way through the form until I get to the Days of Week checklist; then I must use a mouse. Accessibility probably isn't a huge priority rn, but would be nice to revisit this at some time.

Fantastic work! Also great job leaving comments/screenshots throughout the PR - that made it much easier to review.

kylecarbs reacted with hooray emoji

@greyscaled
Copy link
ContributorAuthor

keyboard shortcut my way through the form until I get to the Days of Week checklist; then I must use a mouse.

Interesting, I wonder why that is - I will look into this and see if I can figure it out. If I get stuck spinning my wheels, I'll log a ticket and move on.

The code I was using was adapted from:https://v4.mui.com/components/checkboxes/#checkboxes-with-formgroup which means their example must not be accessible either (odd).

Either way, thank you for finding this, that's a wonderful thing to test/review!!!!

Kira-Pilot reacted with thumbs up emoji

@greyscaledgreyscaled merged commit7c59ec4 intomainMay 26, 2022
@greyscaledgreyscaled deleted the vapurrmaid/gh-1455/part-3/page branchMay 26, 2022 16:11
kylecarbs pushed a commit that referenced this pull requestJun 10, 2022
Resolves:#1455Resolves:#1456Summary:Adds a page (accessible from Workspace Schedule section on a workspace) to edit a schedule.Impact:General parity with CLI for autostart/autostop: that is you can update your schedule from the UI
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@Kira-PilotKira-PilotKira-Pilot approved these changes

@johnstcnjohnstcnjohnstcn approved these changes

@kylecarbskylecarbskylecarbs approved these changes

@ammarioammarioAwaiting requested review from ammario

Assignees

@greyscaledgreyscaled

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Set Auto-off in the UI Set Auto-on in the UI

6 participants

@greyscaled@kylecarbs@ammario@johnstcn@Kira-Pilot
[8]ページ先頭
©2009-2025 Movatter.jp