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 with bounds #721

Merged
merged 8 commits into from
May 29, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/vx-tooltip/Readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ accepts the following props, and will spread any additional props on the tooltip
| className | string | -- | Adds a class (in addition to `vx-tooltip-portal`) to the tooltip container |
| style | object | -- | Sets / overrides any styles on the tooltip container (including top and left) |
| children | node | -- | Sets the children of the tooltip, i.e., the actual content |
| unstyled | bool | true | Whether the tooltip use styles from the style prop or not |
| unstyled | bool | true | Whether the tooltip should use styles from the style prop or not |

#### TooltipWithBounds

Expand All @@ -86,6 +86,7 @@ component (i.e., ...restProps):
| offsetRight | number | 10 | Vertical offset of the tooltip from the passed `top` value, functions as a vertical padding. |
| style | object | -- | Sets / overrides any styles on the tooltip container (including top and left) |
| children | node | -- | Sets the children of the tooltip, i.e., the actual content |
| unstyled | bool | true | Whether the tooltip should render with default styles or not |
dennisja marked this conversation as resolved.
Show resolved Hide resolved

Note that this component is positioned using a `transform`, so overriding `left` and `top` via
styles may have no effect.
Expand Down
18 changes: 8 additions & 10 deletions packages/vx-tooltip/src/tooltips/TooltipWithBounds.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React from 'react';
import { withBoundingRects } from '@vx/bounds';

import Tooltip from './Tooltip';
import Tooltip, { TooltipProps, defaultStyles } from './Tooltip';

type RectShape = {
top: number;
Expand All @@ -18,14 +18,6 @@ type WithBoundingRectsProps = {
parentRect?: RectShape;
};

type TooltipProps = {
left?: number;
top?: number;
className?: string;
style?: React.CSSProperties;
children?: React.ReactNode;
};

type Props = {
offsetLeft?: number;
offsetTop?: number;
Expand All @@ -42,6 +34,7 @@ function TooltipWithBounds({
getRects,
children,
style,
unstyled,
...otherProps
}: Props) {
let left = initialLeft;
Expand All @@ -64,7 +57,12 @@ function TooltipWithBounds({

return (
<Tooltip
style={{ top: 0, transform: `translate(${left}px, ${top}px)`, ...style }}
style={{
top: 0,
transform: `translate(${left}px, ${top}px)`,
...style,
dennisja marked this conversation as resolved.
Show resolved Hide resolved
...(!unstyled && defaultStyles),
}}
{...otherProps}
>
{children}
Expand Down
20 changes: 19 additions & 1 deletion packages/vx-tooltip/test/TooltipWithBounds.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,25 @@
import { TooltipWithBounds } from '../src';
import React from 'react';
import { mount } from 'enzyme';
Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally we would use shallow over mount, is that possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason using shallow fails. I first suspected the fact that TooltipWithBounds is wrapped by a higher-order component that uses life cycle methods to be the reason but even when I use shallow and enable life cycle methods it still fails.
I have seen an issue pointing out that it might be a problem with HOCs but I'm tired now. I Will read through it tomorrow and see whether I can switch mount to shallow. Its been long ever since I used Enzyme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@williaster I found a workaround to use shallow over mount. Let me know what you think about it

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is perfect! yeah usually you have to use .dive() to traverse as needed to whatever selection you are looking for. Thanks for making the change! 🙏

import { TooltipWithBounds, defaultStyles, Tooltip } from '../src';

describe('<TooltipWithBounds />', () => {
test('it should be defined', () => {
expect(TooltipWithBounds).toBeDefined();
});

it('should render the Tooltip with default styles by default', () => {
const wrapper = mount(<TooltipWithBounds>Hello</TooltipWithBounds>);
const styles = wrapper.find(Tooltip).props().style as any;
Object.entries(defaultStyles).forEach(([key, value]) => {
expect(styles[key]).toBe(value);
});
});

it('should render the tooltip without default styles if unstyled is set to true', () => {
const wrapper = mount(<TooltipWithBounds unstyled>Hello</TooltipWithBounds>);
const styles = wrapper.find(Tooltip).props().style as any;
Object.keys(defaultStyles).forEach(key => {
expect(styles[key]).toBeUndefined();
});
});
});