Skip to content

Fixes #38295 - Make the chart clickable and show tooltip#958

Merged
adamruzicka merged 1 commit into
theforeman:masterfrom
kmalyjur:job-inv-clickable-chart
Jun 16, 2025
Merged

Fixes #38295 - Make the chart clickable and show tooltip#958
adamruzicka merged 1 commit into
theforeman:masterfrom
kmalyjur:job-inv-clickable-chart

Conversation

@kmalyjur

@kmalyjur kmalyjur commented Mar 17, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@adamruzicka

adamruzicka commented Mar 18, 2025

Copy link
Copy Markdown
Contributor

This will probably need some adjustments (maybe for pf5?), when I try this on top of latest master I'm getting this when visiting the page

image

@MariaAga

MariaAga commented Mar 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

@adamruzicka in future, could you attach the whole error? as the screenshot just tells us that there is an error and all of its details are above the text you screenshotted :)
(I'm also looking into this error)
I suggest we pause this fix until we get the charts to pf5 so we can use our time better. (this might happen after removing foreman-js)

@adamruzicka

adamruzicka commented Mar 25, 2025

Copy link
Copy Markdown
Contributor

Right, sorry about that. When I try again there's also this (and nothing else). The errors are interleaved with various other things, I must have scrolled past this one previously, I don't really recall seeing it before

image

@MariaAga

MariaAga commented Mar 25, 2025

Copy link
Copy Markdown
Member

yeah so renderInPortal={false} dealt with that cause then it just didnt render he portal so it couldnt create errors, but I guess there was a reason why things should be in the portal? 🤷

@adamruzicka

adamruzicka commented Mar 27, 2025

Copy link
Copy Markdown
Contributor

At the risk of sounding dumb, what is a portal in this context?

@MariaAga

MariaAga commented Mar 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

No idea, some internal things for charts it seems?
PF docs say:

When renderInPortal is true, rendered tooltips will be wrapped in VictoryPortal and rendered within the Portal element within ChartContainer.

Just know that it causes errors

@kmalyjur
kmalyjur force-pushed the job-inv-clickable-chart branch from af5331a to b794f34 Compare April 23, 2025 12:51
@kmalyjur

kmalyjur commented Apr 23, 2025

Copy link
Copy Markdown
Contributor Author

The chart should be working correctly now.

  • The tooltip is on purpose at the top of the chart, so it doesn't cause any issues and doesn't get cropped.
  • Title in JobInvocationConstants.js changed to title: ' ' to fix this:
image
  • Changes in TemplateInvocation.js were made to fix this:
image

@kmalyjur
kmalyjur force-pushed the job-inv-clickable-chart branch from 6e2c94c to 5e84785 Compare June 10, 2025 11:27
@kmalyjur
kmalyjur force-pushed the job-inv-clickable-chart branch from 5e84785 to 29076ab Compare June 10, 2025 11:35
@kmalyjur

kmalyjur commented Jun 10, 2025

Copy link
Copy Markdown
Contributor Author

@MariaAga I resolved the conflict, could you please re-review it now? 😁

MariaAga
MariaAga requested changes Jun 10, 2025
View reviewed changes

@MariaAga MariaAga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Edge case: If I choose from the chart "failed" and then change the filter in the table to "Succeeded" and then choose from the the chart failed again it doesnt change selection.

MariaAga
MariaAga reviewed Jun 10, 2025
View reviewed changes
Comment thread webpack/JobInvocationDetail/TemplateInvocation.js Outdated
Comment on lines +82 to +85
clearInterval(intervalId);
if (intervalRef.current) {
clearInterval(intervalRef.current);
}

@MariaAga MariaAga Jun 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this file be a part of this pr?

@kmalyjur kmalyjur Jun 13, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, I created a new PR for it: #978

@kmalyjur
kmalyjur force-pushed the job-inv-clickable-chart branch from 29076ab to d2d8148 Compare June 13, 2025 16:03
@kmalyjur

kmalyjur commented Jun 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Edge case: If I choose from the chart "failed" and then change the filter in the table to "Succeeded" and then choose from the the chart failed again it doesnt change selection.

@MariaAga Nice catch, thank you. It's fixed now.

MariaAga
MariaAga approved these changes Jun 13, 2025
View reviewed changes

@MariaAga MariaAga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@kmalyjur kmalyjur mentioned this pull request Jun 16, 2025
Merged
@kmalyjur

kmalyjur commented Jun 16, 2025

Copy link
Copy Markdown
Contributor Author

@adamruzicka could you please merge it?

@adamruzicka
adamruzicka merged commit c42da6c into theforeman:master Jun 16, 2025
16 checks passed
@adamruzicka

adamruzicka commented Jun 16, 2025

Copy link
Copy Markdown
Contributor

Thank you @kmalyjur !

@kmalyjur
kmalyjur deleted the job-inv-clickable-chart branch June 29, 2025 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamruzicka adamruzicka adamruzicka approved these changes
@MariaAga MariaAga MariaAga approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kmalyjur @adamruzicka @MariaAga