-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[UI v2] feat: Start flow runs data table UX #17168
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
Conversation
| export type FlowRunWithFlow = FlowRun & { | ||
| flow: Flow; | ||
| }; |
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.
For context I believe this type is around mostly because the FlowRun model doesn't have flow_name but just flow_id. In cloud we recently added flow_name to the model and got rid of this type and the extra fetching to get the flow.
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.
Do you know if flow_name will be available for OSS soon?
I think this would help a lot in future development
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.
idk that there's plan to do so but I'm sure we can make a plan to add it
| const meta: Meta<typeof FlowRunsDataTable> = { | ||
| title: "Components/FlowRuns/DataTable/FlowRunsDataTable", | ||
| decorators: [routerDecorator], |
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.
Ooo is this the fix for the router issues you mentioned last week?
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.
this is something else.
It just allows us to use router dependent components in stories without breaking,
The one I was mentioning last week is for tests
| id: "deployment", | ||
| header: "Deployment", | ||
| cell: (props) => { | ||
| const deployment = props.getValue() as Deployment; |
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.
Is it possible to make this type safe rather than using as? Maybe a type guard?
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.
edit: I think I've figured it out. Needed to define the table schema to be more specific to the table
| }; | ||
|
|
||
| type FlowRunsDataTableProps = { | ||
| flowRuns: Array<FlowRunWithDeploymentAndFlow | FlowRunWithFlow>; |
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.
FWIW I think the "WithDeployment..." type is around for the same reason as the WithFlow type. Because deployment_name doesn't exist on the flow run model. Just context, not requesting any change.
0ebd483 to
308ca66
Compare
Starts flow runs data table with basic columns. Will incrementally add columns based on original vue component .
Will also incrementally add server-side filters
Checklist
<link to issue>"mint.json.Relates to #15512