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.

Sign up for GitHub

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

Jump to bottom

Orchestrator: Workflow runs #166

Merged
merged 7 commits into from
Dec 18, 2024
Merged

Orchestrator: Workflow runs #166

merged 7 commits into from
Dec 18, 2024

Conversation

LiorSoffer
Copy link
Contributor

@LiorSoffer LiorSoffer commented Dec 12, 2024 •
edited
Loading

resolve FLPATH-1042
image

Screencast.from.2024-12-16.16-51-29.mp4

@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Dec 12, 2024 •
edited
Loading

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-orchestrator workspaces/orchestrator/plugins/orchestrator patch v2.5.0

mareklibra
mareklibra requested changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@mareklibra mareklibra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Works nicely, just a few comments. Thank you :-)

workspaces/orchestrator/plugins/orchestrator/src/components/OrchestratorPage.tsx Outdated Show resolved Hide resolved
workspaces/orchestrator/plugins/orchestrator/src/components/OrchestratorPage.tsx Outdated Show resolved Hide resolved
workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowPage.tsx Outdated Show resolved Hide resolved
workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowRunsTabContentFiltered.tsx Outdated Show resolved Hide resolved
workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowRunsTabContentFiltered.tsx Outdated Show resolved Hide resolved
workspaces/orchestrator/plugins/orchestrator/src/routes.ts Outdated Show resolved Hide resolved
workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowRunsTabContentFiltered.tsx Outdated Show resolved Hide resolved
workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowPage.tsx Outdated Show resolved Hide resolved
workspaces/orchestrator/plugins/orchestrator/src/routes.ts Outdated Show resolved Hide resolved
@christoph-jerolimov christoph-jerolimov changed the title Workflow runs Orchestrator: Workflow runs Dec 14, 2024
LiorSoffer and others added 2 commits December 15, 2024 12:06
@LiorSoffer
fix filter
4bdf56a
@LiorSoffer
v1
b12357e 
Signed-off-by: Lior Soffer <[email protected]>
@LiorSoffer
Copy link
Contributor Author

Hey, thank you for the detailed notes ,working on fixing them all :)

@LiorSoffer
fix
89012f6 
Signed-off-by: Lior Soffer <[email protected]>
@batzionb
Copy link
Contributor

@LiorSoffer
please add screenshots to PR description

@batzionb
Copy link
Contributor

@LiorSoffer
The second line in the tabs toolbar is shorter than the first?

@LiorSoffer
Copy link
Contributor Author

LiorSoffer commented Dec 16, 2024 •
edited
Loading

@ShiranHi PTAL in the PR

@ShiranHi
Copy link

@LiorSoffer could you please provide a screen recording of the flow? A screenshot isn’t very helpful in this case. Thanks :)

@batzionb
Copy link
Contributor

@LiorSoffer
The workflow name column needs to be removed when in WorkflowPage, as shown in Figma
https://www.figma.com/design/XjdTCb4vDQ7L4Jz1TdHKGq/RHDH---Workflow-Orchestrator?node-id=4600-8322&t=fwopNGr3UpxMsJNg-0

@LiorSoffer
Copy link
Contributor Author

@ShiranHi screen recording added :)

@ShiranHi
Copy link

@LiorSoffer looks good. We need to remove the Category column, and increase the ID column width

@LiorSoffer
fixes
f7c1f52 
Signed-off-by: Lior Soffer <[email protected]>
@batzionb
Copy link
Contributor

@LiorSoffer Well done!
I added some comments, and as @ShiranHi mentioned the category column needs to be removed when in workflow page
And since there's plenty of room, ID doesn't need to be minimized

@LiorSoffer
show full ID
55d2396 
Signed-off-by: Lior Soffer <[email protected]>
@LiorSoffer
Copy link
Contributor Author

This is how the ID looks now @ShiranHi
image
image

@ShiranHi
Copy link

@LiorSoffer that's great! One small note, can we bring closer the last two columns to the Status column? It will be nice to keep the same padding between all of the columns

@batzionb
Copy link
Contributor

@LiorSoffer that's great! One small note, can we bring closer the last two columns to the Status column? It will be nice to keep the same padding between all of the columns

We can make status, duration and started take less space, but we can't garantuee the same padding

@batzionb batzionb requested a review from mareklibra December 17, 2024 07:58
@batzionb
Copy link
Contributor

@mareklibra
Can you please take one more look to see your change requests are done?

mareklibra
mareklibra approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@mareklibra mareklibra left a comment

Choose a reason for hiding this comment

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

Nice.
I left just nitpicks, it's up to your decision whether fixing them.

.../orchestrator/plugins/orchestrator/src/components/WorkflowPage/WorkflowDetailsTabContent.tsx
workflowOverviewDTO,
}: Props) => {
const { workflowId, format } = useRouteRefParams(workflowRouteRef);
const workflowFormat = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick (no need to fix): since both the json and yaml are string constants, avoiding the useMemo() will perform faster, it will no effect on re-render at the line 56.

.../orchestrator/plugins/orchestrator/src/components/WorkflowPage/WorkflowDetailsTabContent.tsx
</Grid>
<Grid item>
<InfoCard title="Workflow definition">
<div style={{ height: '600px' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use useStyle() instead

workspaces/orchestrator/plugins/orchestrator/src/components/WorkflowPage/WorkflowPage.tsx
loading,
error,
} = useAsync(() => {
return orchestratorApi.getWorkflowOverview(workflowId);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick (no need to fix):

Suggested change
return orchestratorApi.getWorkflowOverview(workflowId);
useAsync(() => orchestratorApi.getWorkflowOverview(workflowId))

@LiorSoffer
fix type
a071fd9 
Signed-off-by: Lior Soffer <[email protected]>
@batzionb batzionb merged commit f3ace9e into redhat-developer:main Dec 18, 2024
7 checks passed
@LiorSoffer LiorSoffer deleted the workflow-runs branch December 18, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@batzionb batzionb batzionb left review comments

@mareklibra mareklibra mareklibra approved these changes

@gciavarrini gciavarrini Awaiting requested review from gciavarrini gciavarrini is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LiorSoffer @batzionb @ShiranHi @mareklibra