FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(charts): Adjust pf-core vars & add tooltip examples by dlabrecq · Pull Request #2497 · patternfly/patternfly-react · GitHub

fix(charts): Adjust pf-core vars & add tooltip examples - #2497

Merged
redallen merged 1 commit into
patternfly:masterfrom
dlabrecq:2037-tooltips
Jul 11, 2019
Merged

fix(charts): Adjust pf-core vars & add tooltip examples#2497
redallen merged 1 commit into
patternfly:masterfrom
dlabrecq:2037-tooltips

Conversation

dlabrecq commented Jul 10, 2019
edited
Loading

Copy link
Copy Markdown
Member

Adjusted/added pf-core vars to ensure custom tooltip colors are displayed correctly. Previously, the background and label were the same black color, so the text wasn't visible.

We were also applying old, hard coded color properties over the pf-core vars, so the theme had to be cleaned up a bit.

Added the voronoiDimension prop to fix the tooltip hover for our bar chart examples. Then, modified the stack chart example to show how similar tooltips are applied without a voronoi container.

Also modified an area chart example, showing how width can be responsive (per @priley86 's request).

Fixes #2496
Fixes #2482

Copy link
Copy Markdown
Collaborator

PatternFly-React preview: https://patternfly-react-pr-2497.surge.sh

tlabaj requested review from jcaianirh and kmcfaul July 11, 2019 00:17
tlabaj requested a review from karelhala July 11, 2019 00:18
dlabrecq marked this pull request as ready for review July 11, 2019 02:06
dlabrecq requested review from priley86 and removed request for jcaianirh and kmcfaul July 11, 2019 02:46

Copy link
Copy Markdown
Member

This looks good to me and should resolve my default background issue in #2476 (so I'd be able to use the same theme).

The responsive use case you've added here should help. I'd argue we should try to add those examples for test purposes for each Chart in the future w/ the way these svgs are scaling. I think what's potentially still different about my use case in 35 is I'm expected to render around 30 x-axis labels responsively (days of the month). When I compress this chart for mobile, I'm seeing some weirdness with the Voronoi tooltip hover, where if bars become close, the first bar hovered is still showing it's tooltip when I move to hover a second bar. I'm not seeing this same buggy behavior happen when I pass the default ChartTooltip to the ChartBar labelComponent like the example here:
https://formidable.com/open-source/victory/guides/tooltips#simple-tooltips

I will continue to experiment here, but it's just something to test w/ VoronoiContainer in the future. Not sure if you would see the same thing here if trying to render several x-axis labels.

karelhala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looking good. So if I undestand this correctly I will add voronoiDimension to whichever axis I want to?

redallen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Always happy to see cleaner code with functionality intact :)

redallen merged commit e19eb37 into patternfly:master Jul 11, 2019

Copy link
Copy Markdown
Collaborator

Your changes have been released in:

  • @patternfly/react-charts@4.5.2

Thanks for your contribution! 🎉

dlabrecq commented Jul 11, 2019
edited
Loading

Copy link
Copy Markdown
Member Author

Thank you for the reviews!

@karelhala Yes, please use voronoiDimension for your dependent axis. Or, try the approach used by the stack chart example.

@priley86 Although, a voronoi container appears to work with bar charts, it may not be best when wrapped by a stack chart? I've also encountered some (non-mobile) jumpy hovers with the stack example.

You could try working with a basic container, instead? I've also modified the stack chart example to show an alternative way to apply tooltips without voronoi container -- that fixed the tooltip hover behavior.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

Chart tooltip colors are incorrect PF4: Chart tooltip is unresponsive

7 participants


Back | FazBrowse Home | New Git URL