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(tooltip): fallback to left/top positioning, add applyPositionStyle #857

Merged
merged 3 commits into from
Oct 12, 2020

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Oct 8, 2020

🐛 Bug Fix

This partially reverts #828 (which fixed #767) because the re-implementation of in Tooltip to use transform instead of top/left for positioning breaks cases where users themselves set style.transform and would thus be a 💥 breaking change. This was true in the /areas demo:

Before

After

🚀 Enhancements

@visx/tooltip

  • adds a new applyPositionStyle which applies position: absolute so that users who set unstyled=true don't have to set this themselves via a CSS selector 🤦 . In a future breaking change this will be the default behavior.

@hshoff @kristw

React.HTMLProps<HTMLDivElement> &
WithBoundingRectsProps;

function TooltipWithBounds({
children,
Copy link
Collaborator Author

@williaster williaster Oct 8, 2020

Choose a reason for hiding this comment

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

abc ordering

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is abc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry updated

@@ -2,13 +2,26 @@ import React from 'react';
import cx from 'classnames';

export type TooltipProps = {
children?: React.ReactNode;
Copy link
Collaborator Author

@williaster williaster Oct 8, 2020

Choose a reason for hiding this comment

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

abc ordering

@coveralls
Copy link

coveralls commented Oct 8, 2020

Pull Request Test Coverage Report for Build 294468908

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 18 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.6%) to 57.345%

Files with Coverage Reduction New Missed Lines %
packages/visx-tooltip/src/hooks/useTooltip.ts 3 44.44%
packages/visx-xychart/src/components/Tooltip.tsx 3 82.96%
packages/visx-xychart/src/components/series/LineSeries.tsx 5 40.63%
packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx 7 0%
Totals Coverage Status
Change from base Build 119: 0.6%
Covered Lines: 1285
Relevant Lines: 2172

💛 - Coveralls

@williaster williaster merged commit 72dfcf2 into master Oct 12, 2020
@williaster williaster deleted the chris--fix-tooltip-transform branch October 12, 2020 19:19
@williaster williaster added this to the 1.1.0 milestone Oct 17, 2020
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
3 participants