Skip to content

Conversation

@pr-watson
Copy link
Contributor

@pr-watson pr-watson commented Nov 18, 2025

Description

This PR fixes the issue with legend hovering since upgrading to patternfly 6.

Jira link:
RSPEED-2053


Screenshots

Before:

image

After:

image

Checklist ☑️

  • PR only fixes one issue or story
  • Change reviewed for extraneous code
  • UI best practices adhered to
  • Commits squashed and meaningfully named
  • All PR checks pass locally (build, lint, test, E2E)

  • (Optional) QE: Needs QE attention (OUIA changed, perceived impact to tests, no test coverage)
  • (Optional) QE: Has been mentioned
  • (Optional) UX: Needs UX attention (end user UX modified, missing designs)
  • (Optional) UX: Has been mentioned

getInteractiveLegendEvents,
getInteractiveLegendItemStyles,
} from '@patternfly/react-charts/victory';
import { VictoryLegend } from 'victory-legend';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we want to use directly the vicotry-legend as it seems to me we won't have the patternfly customizations (patternfly charts are based on victory). When I check the interactive legend in patternfly docs, ChartLegend and getInteractiveLegendEvents are still used in the code.

I believe it's also the source of difference in the look, notice the bigger gap and missing crossed eye. This is how it looks now:
Screenshot From 2025-11-19 11-46-05

and this is with the previous code (basically the same look as in the patternfly docs):
Screenshot From 2025-11-19 11-46-17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch thanks! I was able to use the patternfly components instead of directly from Victory

@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

❌ Patch coverage is 67.91444% with 60 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (pf6-upgrade@fcb2370). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/Components/LifecycleChart/LifecycleChart.tsx 67.74% 29 Missing and 1 partial ⚠️
...ents/LifecycleChartSystem/LifecycleChartSystem.tsx 67.74% 29 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##             pf6-upgrade     #236   +/-   ##
==============================================
  Coverage               ?   61.52%           
==============================================
  Files                  ?       40           
  Lines                  ?     7412           
  Branches               ?      697           
==============================================
  Hits                   ?     4560           
  Misses                 ?     2823           
  Partials               ?       29           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

})}
legendComponent={
<ChartLegend
name="chart5-ChartLegend"
Copy link
Contributor

@hosekadam hosekadam Nov 24, 2025

Choose a reason for hiding this comment

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

I had a feeling we probably should use styles from getInteractiveLegendItemStyles to reduce complexity. So I tried a small collab with gpt-5.1-codex and here is the result: Only thing we need it to use custom event handling instead of the onLegendClick - this seems buggy in our use case. So then the Chart looks like:

<Chart
        key={renderKey} // Force re-render when renderKey changes
        legendAllowWrap
        ariaDesc="Support timelines of packages and RHEL versions"
        legendComponent={
          <ChartLegend
            name="chart5-ChartLegend"
            events={[
              {
                target: 'data',
                eventHandlers: {
                  onClick: (_event, props) => {
                    handleLegendClick({ index: props.index });
                    return [];
                  },
                },
              },
              {
                target: 'labels',
                eventHandlers: {
                  onClick: (_event, props) => {
                    handleLegendClick({ index: props.index });
                    return [];
                  },
                },
              },
            ]}
            symbolSpacer={1}
            borderPadding={{ top: 12, bottom: 0, left: 10, right: 0 }}
            data={getLegendData()}
            height={50}
            gutter={20}
          />
        }
        legendPosition="bottom-left"
...

events moved to ChartLegend and added padding on the left. Any other change is needed. From my testing, this seems to be working. Could you please check if that works also for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh, I see. The onHover doesn't work, I forgot it should work.... Sorry

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to get it to work, only benefit I see is missing calculation of the legend symbol (if there should be square or eyeslash, it's handled by patternfly logic). Otherwise, it uses manual handling of the actions. You can check, but your solution is also nice!

onhover.txt
(It's a git diff output, GitHub just doesn't allow .diff files, so I used .txt. You can apply it using git apply onhover.txt in the repo.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants