8000 Chart tooltips by dlabrecq · Pull Request #7149 · patternfly/patternfly · GitHub
[go: up one dir, main page]

Skip to content

Chart tooltips#7149

Merged
dlabaj merged 1 commit intopatternfly:mainfrom
dlabrecq:chart-tooltips
Oct 18, 2024
Merged

Chart tooltips#7149
dlabaj merged 1 commit intopatternfly:mainfrom
dlabrecq:chart-tooltips

Conversation

@dlabrecq
Copy link
Member
@dlabrecq dlabrecq commented Oct 16, 2024

Charts should have rounded corners for PF v6

Fixes #7148

Screenshot 2024-10-16 at 4 26 39 PM

@dlabrecq dlabrecq requested a review from mcoker October 16, 2024 20:23
@patternfly-build
Copy link
Collaborator
patternfly-build commented Oct 16, 2024

Copy link
Contributor
@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM! Left a comment in case it's helpful. Feel free to merge whenever you think it's ready or re-request a review if you push updates and you want another review 👍


// Tooltip
--#{$chart}-tooltip--corner-radius: 0;
--#{$chart}-tooltip--corner-radius: 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you have a good way to do this on the react side, but I wonder if you could use the --pf-t--global--border--radius--small token here (and for --#{$chart}-tooltip--flyoutStyle--corner-radius below), and strip the unit in react-charts when you reference chart_tooltip_corner_radius.value and chart_tooltip_flyoutStyle_corner_radius.value?

FWIW, here's the tooltip style that references that same token

--#{$tooltip}__content--BorderRadius: var(--pf-t--global--border--radius--small);

Copy link
Member Author
@dlabrecq dlabrecq Oct 17, 2024

Choose a reason for hiding this comment

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

I don't think it's a good idea to include units here. This introduces a mix of numbers and strings in react-tokens, which is confusing because Victory can't use units. Not only would we need to strip the units, but anyone creating a custom tooltip would likely need to do the same.

Copy link
Contributor
@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@dlabaj dlabaj merged commit 42db5f3 into patternfly:main Oct 18, 2024
@patternfly-build
Copy link
Collaborator

🎉 This PR is included in version 6.0.0-prerelease.16 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chart tooltips

4 participants

0