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 TooltipWithBounds positioning #828

Merged
merged 5 commits into from
Oct 7, 2020
Merged

Conversation

heyanurag
Copy link
Contributor

@heyanurag heyanurag commented Oct 1, 2020

🐛 Bug Fix

@coveralls
Copy link

coveralls commented Oct 1, 2020

Pull Request Test Coverage Report for Build 293983229

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 36 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+2.3%) to 56.685%

Files with Coverage Reduction New Missed Lines %
packages/visx-pattern/src/patterns/Lines.tsx 1 85.0%
packages/visx-xychart/src/classes/DataRegistry.ts 1 78.26%
packages/visx-xychart/src/components/XYChart.tsx 2 92.5%
packages/visx-xychart/src/hooks/useScales.ts 3 73.08%
packages/visx-responsive/src/enhancers/withParentSize.tsx 5 58.06%
packages/visx-xychart/src/components/series/LineSeries.tsx 5 41.38%
packages/visx-xychart/src/hooks/useDataRegistry.ts 6 45.45%
packages/visx-responsive/src/components/ParentSize.tsx 13 0.0%
Totals Coverage Status
Change from base Build 24: 2.3%
Covered Lines: 1250
Relevant Lines: 2133

💛 - Coveralls

@heyanurag
Copy link
Contributor Author

@williaster Was this correct or not? Did I do something wrong? Please reply.

@williaster
Copy link
Collaborator

hey @singhanurag05 thanks for the fix! just to check I pulled this branch and played with it locally, then realized that the ideal solution is a bit more complicated.

I opened #831 with what I think is the needed fix. If you'd like to implement this yourself for more coding experience I'm happy to close it if you make those updates! Otherwise we can close this PR and use that one.

@heyanurag
Copy link
Contributor Author

I had thought of the exact same thing! please let me do this @williaster

@williaster
Copy link
Collaborator

williaster commented Oct 1, 2020

@singhanurag05 great! One comment on my changes: let's not set position: 'absolute' by default as this would be a breaking change. This means that when you set unstyled, you must set .visx-tooltip { position: absolute; } for correct positioning but we want to improve this in another PR in a non-breaking way.

@heyanurag
Copy link
Contributor Author

@williaster Sorry I didn't get you. Can you please explain?

@williaster
Copy link
Collaborator

In my PR I made the change to remove position: absolute from defaultStyles, and always set it in Tooltip. This is technically needed for the tooltip positioning to work properly (without it, consumers who set unstyled must set their own css style rule .visx-tooltip { position: absolute; }), but it's technically a breaking change (which would require a 2.0 release).

so if you mirror the changes in my PR, I just meant to not make that change (i.e., leave position: absolute in defaultStyles, and don't apply it in Tooltip). make sense?

@heyanurag
Copy link
Contributor Author

Absolutely yes! i'll make the changes in sometime. Thanks for the help!

@heyanurag heyanurag changed the title remove line unstyled={unstyled} in TooltipWithBounds.tsx fix TooltipWithBounds positioning Oct 3, 2020
Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

Looks good to me! had one last suggestion but then I think this is good to go.

packages/visx-tooltip/src/tooltips/Tooltip.tsx Outdated Show resolved Hide resolved
Co-authored-by: Chris Williams <williaster@users.noreply.github.com>
@heyanurag
Copy link
Contributor Author

Thank you! Hope to contribute further.

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.

TooltipWithBounds bug
4 participants