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

feat(scheduled builds): Add UI for scheduled builds proposal #660

Merged
merged 34 commits into from
Jun 8, 2023

Conversation

JayCeeJr
Copy link
Contributor

@JayCeeJr JayCeeJr commented May 5, 2023

Empty

Screenshot 2023-05-25 at 12 52 19

Add Form

Screenshot 2023-05-25 at 12 52 29

List

Screenshot 2023-05-25 at 12 53 09

@wass3rw3rk
Copy link
Member

is there a visual mock for what this is supposed to look like?

@JayCeeJr
Copy link
Contributor Author

is there a visual mock for what this is supposed to look like?

I added some screenshots to the description

@JayCeeJr JayCeeJr marked this pull request as ready for review May 26, 2023 17:46
@JayCeeJr JayCeeJr requested a review from a team as a code owner May 26, 2023 17:46
Copy link
Contributor

@plyr4 plyr4 left a comment

Choose a reason for hiding this comment

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

awesome work, looks great so far.

just a couple tweaks, and a few comments/questions.

src/elm/Vela.elm Outdated Show resolved Hide resolved
src/elm/Pages/Schedules/Update.elm Outdated Show resolved Hide resolved
src/elm/Pages/Schedules/Form.elm Outdated Show resolved Hide resolved
src/elm/Pages/Schedules/Form.elm Outdated Show resolved Hide resolved
src/elm/Help/Commands.elm Outdated Show resolved Hide resolved
src/elm/Help/Commands.elm Outdated Show resolved Hide resolved
src/elm/Api.elm Outdated Show resolved Hide resolved
src/elm/Api.elm Outdated Show resolved Hide resolved
src/elm/Pages.elm Show resolved Hide resolved
src/elm/Pages/Schedules/Update.elm Outdated Show resolved Hide resolved
@wass3rw3rk wass3rw3rk changed the title Feature(scheduled builds): Add UI for scheduled builds proposal feat(scheduled builds): Add UI for scheduled builds proposal May 30, 2023
Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

feedback inline; looks to be working fine. thanks for the work!

a couple of items:

docker compose addition

we should add VELA_SCHEDULE_ALLOWLIST: '*' to the docker compose for the server here, just as we did in go-vela/server

design feedback for add/edit schedule

tl;dr: no edits requested at this time, although it would be neat if some of the following could make its way in prior to the release.

i'm not sure if there are additional plans for the add/edit view for a schedule, but it's a bit bare and geared towards advanced users at the moment. we can probably agree that cron is not the most approachable format for scheduling to start with, so i was really hoping that we can be a bit more helpful here. you will see that a few online editors stuff the raw cron expression under an "Advanced" section while the initial exposure is something more approachable (or there are helpful annotations or links to dedicated cron tools). here is some quick reference material:

not saying any of these are perfect but there are some themes/ideas. i think it boils down to these points for the current implementation:

  • what is "Entry"? What am I looking at? *
  • either a link to an editor or more inline explanation what each field represents, etc *
  • quick views to edit common daily, weekly, hourly, etc schedules in more human way
  • a cron expression to word translator would be nice
  • something that tells you when the current expression would run next (taking into account server time zone) *
  • calendar view would be neat (but value-add is probably diminishing there?)

maybe we can add some of the items at minimum and let user feedback guide the rest. i put a * next to the items that i view as lowest effort (honoring server timezone could be a little tricky?).

thoughts?

src/elm/Pages/Schedules/Form.elm Outdated Show resolved Hide resolved
src/elm/Pages/Schedules/Form.elm Outdated Show resolved Hide resolved
src/elm/Pages/Schedules/Model.elm Outdated Show resolved Hide resolved
src/elm/Pages/Schedules/Update.elm Outdated Show resolved Hide resolved
src/elm/Pages/Schedules/View.elm Outdated Show resolved Hide resolved
src/elm/Help/Commands.elm Outdated Show resolved Hide resolved
src/elm/Pages/Schedules/Form.elm Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@JayCeeJr
Copy link
Contributor Author

JayCeeJr commented May 31, 2023

I changed entry to "Cron Expression" and added a help link to https://crontab.guru/
Maybe it would be better to add the cron expression explanation to the vela docs, and put links to the other cron sites there?
Screenshot 2023-05-31 at 12 03 23
Screenshot 2023-05-31 at 12 03 10

@wass3rw3rk
Copy link
Member

wass3rw3rk commented May 31, 2023

@JayCeeJr thanks for the edits.. agree - it should probably have a place in the docs as well but keeping it closer to the context (where it is being used) seems prudent.

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

missing cypress tests still

@JayCeeJr JayCeeJr requested review from wass3rw3rk and plyr4 June 8, 2023 14:56
src/elm/Main.elm Outdated Show resolved Hide resolved
src/elm/Main.elm Outdated Show resolved Hide resolved
src/elm/Main.elm Outdated Show resolved Hide resolved
src/elm/Main.elm Outdated Show resolved Hide resolved
src/elm/Main.elm Outdated Show resolved Hide resolved
src/elm/Main.elm Outdated Show resolved Hide resolved
src/elm/Main.elm Outdated Show resolved Hide resolved
@JayCeeJr JayCeeJr requested a review from plyr4 June 8, 2023 18:15
@plyr4
Copy link
Contributor

plyr4 commented Jun 8, 2023

thank you for the tweaks, looks good

@wass3rw3rk wass3rw3rk merged commit 235ff17 into go-vela:main Jun 8, 2023
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.

4 participants