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

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

Merged
merged 1 commit into from Jul 11, 2019

Conversation

@dlabrecq
Copy link
Member

dlabrecq commented Jul 10, 2019

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

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 10, 2019

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

@dlabrecq dlabrecq force-pushed the dlabrecq:2037-tooltips branch from 671e7ed to 06552df Jul 10, 2019
@tlabaj tlabaj requested review from kmcfaul and jcaianirh Jul 11, 2019
@tlabaj tlabaj requested a review from karelhala Jul 11, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:2037-tooltips branch from 06552df to 551b311 Jul 11, 2019
@dlabrecq dlabrecq marked this pull request as ready for review Jul 11, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:2037-tooltips branch from 551b311 to 75a96fb Jul 11, 2019
@dlabrecq dlabrecq requested review from priley86 and removed request for jcaianirh and kmcfaul Jul 11, 2019
@priley86

This comment has been minimized.

Copy link
Member

priley86 commented Jul 11, 2019

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.

Copy link
Contributor

karelhala left a comment

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

Copy link
Contributor

redallen left a comment

Always happy to see cleaner code with functionality intact :)

@redallen redallen merged commit e19eb37 into patternfly:master Jul 11, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 11, 2019

Your changes have been released in:

  • @patternfly/react-charts@4.5.2

Thanks for your contribution! 🎉

@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented Jul 11, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.