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

[UI v2] feat: Adding concurrency-limit-task-run-card component #16444

Merged
merged 1 commit into from
Dec 26, 2024

Conversation

devinvillarosa
Copy link
Contributor

@devinvillarosa devinvillarosa commented Dec 18, 2024

  1. Create route placeholder for /runs/task-run/$id
  2. Ports over @/utilities/seconds
  3. Adds RunCard component to be used

Screenshot 2024-12-21 at 1 08 54 PM
Screenshot 2024-12-21 at 1 08 59 PM

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

Relates to #15512

@github-actions github-actions bot added the ui-replatform Related to the React UI rewrite label Dec 18, 2024
@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch from e08e3bc to 1b0e12d Compare December 18, 2024 22:17
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ported over from other repo

@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch from 1b0e12d to f30d85b Compare December 18, 2024 22:27
@devinvillarosa devinvillarosa marked this pull request as ready for review December 18, 2024 22:32
@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch 2 times, most recently from 887d012 to 4e28f9e Compare December 20, 2024 03:31
@aaazzam
Copy link
Collaborator

aaazzam commented Dec 20, 2024

@devinvillarosa IIRC was have "RunCards" like this all over. What's "concurrency"-y about this component?

@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch from 4e28f9e to 4424975 Compare December 20, 2024 22:39
@devinvillarosa
Copy link
Contributor Author

@devinvillarosa IIRC was have "RunCards" like this all over. What's "concurrency"-y about this component?

ahh makes sense.
I think this may be the first one for re-platforming.

I don't mind revisiting this and make a more generic RunCard UI component to be used instead once we tackle it in another page

@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch 2 times, most recently from d577f94 to e08d97f Compare December 21, 2024 21:12
@devinvillarosa
Copy link
Contributor Author

@devinvillarosa IIRC was have "RunCards" like this all over. What's "concurrency"-y about this component?

Actually, I see what you mean. I updated the component to be just RunCard where it can render for task runs or flow runs.
But the flow runs will probably need to re-visit this component when we develop the Flow Run card to add additional features like onSelect, params, etc

Screenshot 2024-12-21 at 1 08 54 PM
Screenshot 2024-12-21 at 1 08 59 PM

@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch 2 times, most recently from fe9e370 to 6f74262 Compare December 24, 2024 16:12
@@ -0,0 +1,143 @@
import { components } from "@/api/prefect";

export const MOCK_TASK_RUN: components["schemas"]["TaskRun"] = {
Copy link
Member

Choose a reason for hiding this comment

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

It'd be cool to use @faker-js/faker to generate random task runs for display. I've been doing that for deployments, and it's helped me uncover some edge cases when working with display components like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I've started to centralize some reusable schemas using faker in @/storybook/utils/mocks

Comment on lines 63 to 78
if (!flow.id || !flowRun.id) {
throw new Error("'id' field expected");
}
Copy link
Member

Choose a reason for hiding this comment

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

Task runs aren't guaranteed to have a parent flow run, so we'll need to handle this case without erroring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I addressed this in the latest push

ui-v2/src/utilities/seconds.ts Outdated Show resolved Hide resolved
@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch 2 times, most recently from 432e981 to ac0357c Compare December 25, 2024 05:47
@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch 3 times, most recently from d0825b2 to 9426006 Compare December 26, 2024 05:50
@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch from 9426006 to 8b72c80 Compare December 26, 2024 16:37
@devinvillarosa devinvillarosa force-pushed the concurrency-limitr-task-run-card branch from 8b72c80 to c7fb16b Compare December 26, 2024 18:21
Comment on lines +39 to +41
flowRun?: components["schemas"]["FlowRun"];
/** If task run is included, uses fields from task run over flow run */
taskRun?: components["schemas"]["TaskRun"];
Copy link
Member

Choose a reason for hiding this comment

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

I think we may want to make this parentRun and run in the future to be more flexible and handle more cases, but this is good for now since it has parity with the current UI.

@devinvillarosa devinvillarosa merged commit 2d9450b into main Dec 26, 2024
8 checks passed
@devinvillarosa devinvillarosa deleted the concurrency-limitr-task-run-card branch December 26, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui-replatform Related to the React UI rewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants