-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
VTAdmin: Initiate workflow details tab #16570
Conversation
Signed-off-by: Noble Mittal <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
2133c22
to
b1a2287
Compare
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
b1a2287
to
3449de3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16570 +/- ##
==========================================
- Coverage 68.96% 68.92% -0.04%
==========================================
Files 1562 1562
Lines 200730 200855 +125
==========================================
+ Hits 138430 138436 +6
- Misses 62300 62419 +119 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
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.
Excellent work. Only issue I see atm is whether we load the tab upfront or lazily.
@@ -74,6 +75,7 @@ export const Workflow = () => { | |||
<ContentContainer> | |||
<TabContainer> | |||
<Tab text="Streams" to={`${url}/streams`} count={streams.length} /> | |||
<Tab text="Details" to={`${url}/details`} /> |
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 we load the Details info all the time? If so, we should only load on clicking the tab, since there are a large number of shards that could take a lot of time.
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.
We are loading the details info when we click on the tab.
We should figure out a way to dynamically load some of the details info like logs or streams in clusters with a very large number of shards. We can do that later once we have some experience in the field with the new UI. Just mentioning it here for future reference. |
return rows.map((row) => { | ||
const reverseWorkflow = row.reverseWorkflow; | ||
return ( | ||
<tr> |
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.
You'll want to supply a key here, since it's a mapped react component:
<tr> | |
<tr key={row.workflowData.workflow.name}> |
or another unique identifier!
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.
Done.
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.
Awesome, thank you!
Signed-off-by: Noble Mittal <[email protected]>
Description
This PR adds a new tab
Details
which extracts theWorkflow Show
andWorkflow Status
output into structured UI elements. TheDetails
tab includesRelated Issue(s)
Checklist
Deployment Notes