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

Schedule Frequency as JSON #1598

Merged
merged 9 commits into from
Aug 30, 2023
Merged

Schedule Frequency as JSON #1598

merged 9 commits into from
Aug 30, 2023

Conversation

Alex-Tideman
Copy link
Contributor

@Alex-Tideman Alex-Tideman commented Aug 29, 2023

Description & motivation 💭

Due to the infinite combination of possible schedules, instead of trying and failing to use a human readable label, show the calendar and interval as JSON.

Rearrange layout of table row and Schedule view to make the JSON easily viewable and not take up too much space.

Screenshots (if applicable) 📸

Screen Shot 2023-08-29 at 2 25 43 PM Screen Shot 2023-08-29 at 2 27 43 PM Screen Shot 2023-08-29 at 2 27 54 PM

Design Considerations 🎨

Testing 🧪

How was this tested 👻

  • Manual testing
  • E2E tests added
  • Unit tests added

Steps for others to test: 🚶🏽‍♂️🚶🏽‍♀️

Checklists

Draft Checklist

Merge Checklist

Issue(s) closed

Docs

Any docs updates needed?

@Alex-Tideman Alex-Tideman requested review from rossedfort and a team as code owners August 29, 2023 19:28
@vercel
Copy link

vercel bot commented Aug 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
holocene ⬜️ Ignored (Inspect) Visit Preview Aug 30, 2023 7:39pm

@yiminc
Copy link
Member

yiminc commented Aug 30, 2023

Instead of use 'Frequency', should just be 'Schedule Spec'.

Copy link
Contributor

@laurakwhit laurakwhit left a comment

Choose a reason for hiding this comment

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

Just some comments r.e. potential UI improvements, otherwise lgtm!


const intervalSecs = interval?.interval as string;
const phaseSecs = interval?.phase as string;
export let inline = false;
</script>

<div class="flex flex-col {$$props.class}">
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a max height here on xl screens?

Suggested change
<div class="flex flex-col {$$props.class}">
<div class="flex flex-col h-full xl:max-h-96 overflow-scroll {$$props.class}">
Screenshot 2023-08-30 at 12 03 15 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah I want to show the whole thing, and with this layout we get quite a bit of height on the right side so there's no reason to not take it up.

import ScheduleFrequency from './schedule-frequency.svelte';



import type { IntervalSpec } from '$types';

export let calendar: StructuredCalendar | undefined = undefined;
export let interval: IntervalSpec | undefined = undefined;
</script>

<Panel>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we want this Panel to come after Upcoming Runs (or above Recent Runs) instead of Advanced Settings on smaller screens

Screenshot 2023-08-30 at 11 43 58 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah makes sense

@@ -87,6 +83,17 @@
{/each}
</td>
</TableRow>
<TableRow>
Copy link
Contributor

@laurakwhit laurakwhit Aug 30, 2023

Choose a reason for hiding this comment

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

Do users need to see the entire Schedule Spec in this view? Or would even something smaller like this be adequate? Accessibility wise, it's not great that this is it's own separate row with no relation to the last.

Screenshot 2023-08-30 at 11 58 01 AM

Or, at the very least, maybe we could add a !border-t-0 to remove the top border so the row is more visually connected?

Screenshot 2023-08-30 at 12 01 51 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's really hard to read the spec when it's small, so we wanted to give it as much width as possible. I'll remove the top border though.

@Alex-Tideman Alex-Tideman merged commit a7248ac into main Aug 30, 2023
10 checks passed
@Alex-Tideman Alex-Tideman deleted the schedule-frequency-fix branch August 30, 2023 19:54
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.

3 participants