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

feat(dashboard): varName of TaskDef in WfSpec #846

Merged
merged 20 commits into from
Aug 6, 2024

Conversation

hazimoarafa
Copy link
Member

Before:
image

After:
image

@hazimoarafa hazimoarafa marked this pull request as draft May 30, 2024 23:20
@hazimoarafa hazimoarafa added the dashboard LittleHorse administrative dashboard. label Jun 4, 2024
@hazimoarafa hazimoarafa self-assigned this Jun 4, 2024
@hazimoarafa hazimoarafa reopened this Jun 15, 2024
@hazimoarafa
Copy link
Member Author

hazimoarafa commented Jun 15, 2024

This is not the correct implementation of getting the taskDef name. This uses the TaskNode instead of the TaskDef to get the variable name. Needs to be fixed

@hazimoarafa
Copy link
Member Author

hazimoarafa commented Jul 17, 2024

image Works now.

The only issue is that it makes a call for every TaskDef needed, there is no straightforward way to fetch the TaskDef only when the tooltip is open. We can either keep this PR open or merge this and open an issue to refactor for performance.

There is caching enabled so it wont re-fetch on re-render but that's not enough

@hazimoarafa hazimoarafa marked this pull request as ready for review July 17, 2024 21:34
@hazimoarafa hazimoarafa requested a review from mijailr August 5, 2024 23:48
Copy link
Contributor

@mijailr mijailr left a comment

Choose a reason for hiding this comment

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

The only issue is that it makes a call for every TaskDef needed, there is no straightforward way to fetch the TaskDef only when the tooltip is open. We can either keep this PR open or merge this and open an issue to refactor for performance.

You can prevent this by using the selected boolean available at the node, so you make the call only when selected === true

Thanks for the suggestion @mijailr that was good one.
@hazimoarafa hazimoarafa requested a review from mijailr August 6, 2024 20:10
@hazimoarafa
Copy link
Member Author

The only issue is that it makes a call for every TaskDef needed, there is no straightforward way to fetch the TaskDef only when the tooltip is open. We can either keep this PR open or merge this and open an issue to refactor for performance.

You can prevent this by using the selected boolean available at the node, so you make the call only when selected === true

Good thinking, I have implemented that.

@hazimoarafa hazimoarafa merged commit a576cc4 into master Aug 6, 2024
15 checks passed
@hazimoarafa hazimoarafa deleted the feat/dashbaord/varName-on-taskDef branch August 6, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dashboard LittleHorse administrative dashboard.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants