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.

Sign up for GitHub

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

Jump to bottom

minor fix to <Text> and re-enable unit tests. #790

Merged
merged 2 commits into from
Aug 27, 2020
Merged

minor fix to <Text> and re-enable unit tests. #790

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7225f0
fix: text rendering with invalid x or y
kristw Aug 26, 2020
f20f81e
fix: address comments
kristw Aug 26, 2020
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
5 changes: 2 additions & 3 deletions packages/vx-text/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,10 @@
"homepage": "https://github.com/hshoff/vx#readme",
"dependencies": {
"@types/classnames": "^2.2.9",
"@types/lodash": "^4.14.146",
"@types/lodash.memoize": "^4.1.6",
"@types/react": "*",
"classnames": "^2.2.5",
"lodash": "^4.17.15",
"prop-types": "^15.7.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

we actually need this because we have a babel plugin that adds proptypes based on the types defined for a component.

this is useful for consumers I think (proptypes complain in dev / in the console), but @hshoff also was surprised by this recently so maybe we could consider removing it througout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok!

"lodash.memoize": "^4.1.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've gone through and done this before but had forgot this was intentional, see #66

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh ok sure. will revert.

"reduce-css-calc": "^1.3.0"
},
"peerDependencies": {
Expand Down
22 changes: 17 additions & 5 deletions packages/vx-text/src/Text.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ function isNumber(val: unknown): val is number {
return typeof val === 'number';
}

function isValidXOrY(xOrY: string | number | undefined) {
return (
// number that is not NaN or Infinity
(typeof xOrY === 'number' && Number.isFinite(xOrY)) ||
// for percentage
typeof xOrY === 'string'
);
}

interface WordWithWidth {
word: string;
width: number;
Expand All @@ -24,7 +33,7 @@ type SVGTextProps = React.SVGAttributes<SVGTextElement>;
type OwnProps = {
/** className to apply to the SVGText element. */
className?: string;
/** Whether to scale the fontSize to accomodate the specified width. */
/** Whether to scale the fontSize to accommodate the specified width. */
scaleToFit?: boolean;
/** Rotational angle of the text. */
angle?: number;
Expand Down Expand Up @@ -176,6 +185,11 @@ class Text extends React.Component<TextProps, TextState> {
const { wordsByLines } = this.state;
const { x, y } = textProps;

// Cannot render <text> if x or y is invalid
if (!isValidXOrY(x) || !isValidXOrY(y)) {
return <svg ref={innerRef} x={dx} y={dy} fontSize={textProps.fontSize} style={SVG_STYLE} />;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you think we should return null or an svg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about return null but think if at least having the svg may keep the layout from changing if consumer depends on having some element there.

}

let startDy: string | undefined;
if (verticalAnchor === 'start') {
startDy = reduceCSSCalc(`calc(${capHeight})`);
Expand All @@ -187,7 +201,6 @@ class Text extends React.Component<TextProps, TextState> {
startDy = reduceCSSCalc(`calc(${wordsByLines.length - 1} * -${lineHeight})`);
}

let transform: string | undefined;
const transforms = [];
if (isNumber(x) && isNumber(y) && isNumber(width) && scaleToFit && wordsByLines.length > 0) {
const lineWidth = wordsByLines[0].width || 1;
Expand All @@ -200,9 +213,8 @@ class Text extends React.Component<TextProps, TextState> {
if (angle) {
transforms.push(`rotate(${angle}, ${x}, ${y})`);
}
if (transforms.length > 0) {
transform = transforms.join(' ');
}

const transform = transforms.length > 0 ? transforms.join(' ') : undefined;

return (
<svg ref={innerRef} x={dx} y={dy} fontSize={textProps.fontSize} style={SVG_STYLE}>
Expand Down
2 changes: 1 addition & 1 deletion packages/vx-text/src/util/getStringWidth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import memoize from 'lodash/memoize';
import memoize from 'lodash.memoize';

const MEASUREMENT_ELEMENT_ID = '__react_svg_text_measurement_id';

Expand Down
117 changes: 67 additions & 50 deletions packages/vx-text/test/Text.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import { shallow } from 'enzyme';
import { shallow, mount } from 'enzyme';
import { Text, getStringWidth } from '../src';
import { addMock, removeMock } from './svgMock';

describe('getStringWidth()', () => {
it('should be defined', () => {
Expand All @@ -11,6 +12,9 @@ describe('getStringWidth()', () => {
// TODO: Fix tests (jsdom does not support getComputedTextLength() or getBoundingClientRect()). Maybe use puppeteer

describe('<Text />', () => {
beforeEach(addMock);
afterEach(removeMock);

it('should be defined', () => {
expect(Text).toBeDefined();
});
Expand All @@ -20,68 +24,81 @@ describe('<Text />', () => {
expect(() => shallow(<Text>Hi</Text>)).not.toThrow();
});

// it('Does not wrap long text if enough width', () => {
// const wrapper = shallow(
// <Text width={300} style={{ fontFamily: 'Courier' }}>This is really long text</Text>
// );
it('Does not wrap long text if enough width', () => {
const wrapper = shallow<Text>(
<Text width={300} style={{ fontFamily: 'Courier' }}>
This is really long text
</Text>,
);

// expect(wrapper.instance().state.wordsByLines.length).toEqual(1);
// });
expect(wrapper.instance().state.wordsByLines).toHaveLength(1);
});

// it('Wraps long text if not enough width', () => {
// const wrapper = shallow(
// <Text width={200} style={{ fontFamily: 'Courier' }}>This is really long text</Text>
// );
it('Wraps text if not enough width', () => {
const wrapper = shallow<Text>(
<Text width={200} style={{ fontFamily: 'Courier' }}>
This is really long text
</Text>,
);

// expect(wrapper.instance().state.wordsByLines.length).toEqual(2);
// });
expect(wrapper.instance().state.wordsByLines).toHaveLength(2);
});

// it('Wraps long text if styled but would have had enough room', () => {
// const wrapper = shallow(
// <Text width={300} style={{ fontSize: '2em', fontFamily: 'Courier' }}>This is really long text</Text>
// );
it('Does not wrap text if there is enough width', () => {
const wrapper = shallow<Text>(
<Text width={300} style={{ fontSize: '2em', fontFamily: 'Courier' }}>
This is really long text
</Text>,
);

// expect(wrapper.instance().state.wordsByLines.length).toEqual(2);
// });
expect(wrapper.instance().state.wordsByLines).toHaveLength(1);
});

// it('Does not perform word length calculation if width or scaleToFit props not set', () => {
// const wrapper = shallow(
// <Text>This is really long text</Text>
// );
it('Does not perform word length calculation if width or scaleToFit props not set', () => {
const wrapper = shallow<Text>(<Text>This is really long text</Text>);

// expect(wrapper.instance().state.wordsByLines.length).toEqual(1);
// expect(wrapper.instance().state.wordsByLines[0].width).toEqual(undefined);
// });
expect(wrapper.instance().state.wordsByLines).toHaveLength(1);
expect(wrapper.instance().state.wordsByLines[0].width).toBeUndefined();
});

// it('Render 0 success when specify the width', () => {
// const wrapper = render(
// <Text x={0} y={0} width={30}>{0}</Text>
// );
it('Render 0 success when specify the width', () => {
const wrapper = mount(
<Text x={0} y={0} width={30}>
0
</Text>,
);

// expect(wrapper.text()).toContain('0');
// });
console.log('wrapper', wrapper.text());
expect(wrapper.text()).toContain('0');
});

// it('Render 0 success when not specify the width', () => {
// const wrapper = render(
// <Text x={0} y={0}>{0}</Text>
// );
it('Render 0 success when not specify the width', () => {
const wrapper = mount(
<Text x={0} y={0}>
0
</Text>,
);

// expect(wrapper.text()).toContain('0');
// });
expect(wrapper.text()).toContain('0');
});

// it('Render text when x or y is a percentage', () => {
// const wrapper = render(
// <Text x="50%" y="50%">anything</Text>
// );
it('Render text when x or y is a percentage', () => {
const wrapper = mount(
<Text x="50%" y="50%">
anything
</Text>,
);

// expect(wrapper.text()).toContain('anything');
// });
expect(wrapper.text()).toContain('anything');
});

// it("Don't Render text when x or y is NaN ", () => {
// const wrapperNan = render(
// <Text x={NaN} y={10}>anything</Text>
// );
it("Don't Render text when x or y is NaN", () => {
const wrapperNan = mount(
<Text x={NaN} y={10}>
anything
</Text>,
);

// expect(wrapperNan.text()).not.toContain('anything');
// });
expect(wrapperNan.text()).not.toContain('anything');
});
});
25 changes: 25 additions & 0 deletions packages/vx-text/test/svgMock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// @ts-ignore
let originalFn: typeof SVGElement.prototype.getComputedTextLength;

/**
* JSDom does not implement getComputedTextLength()
* so this function add mock implementation for testing.
*/
export function addMock() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

// @ts-ignore
originalFn = SVGElement.prototype.getComputedTextLength;

// @ts-ignore
SVGElement.prototype.getComputedTextLength = function getComputedTextLength() {
// Make every character 10px wide
return (this.textContent?.length ?? 0) * 10;
};
}

/**
* Remove mock from addMock()
*/
export function removeMock() {
// @ts-ignore
SVGElement.prototype.getComputedTextLength = originalFn;
}
12 changes: 12 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2836,6 +2836,18 @@
resolved "https://registry.yarnpkg.com/@types/json-schema/-/json-schema-7.0.4.tgz#38fd73ddfd9b55abb1e1b2ed578cb55bd7b7d339"
integrity sha512-8+KAKzEvSUdeo+kmqnKrqgeE+LcA0tjYWFY7RPProVYwnqDjukzO+3b6dLD56rYX5TdWejnEOLJYOIeh4CXKuA==

"@types/lodash.memoize@^4.1.6":
version "4.1.6"
resolved "https://registry.yarnpkg.com/@types/lodash.memoize/-/lodash.memoize-4.1.6.tgz#3221f981790a415cab1a239f25c17efd8b604c23"
integrity sha512-mYxjKiKzRadRJVClLKxS4wb3Iy9kzwJ1CkbyKiadVxejnswnRByyofmPMscFKscmYpl36BEEhCMPuWhA1R/1ZQ==
dependencies:
"@types/lodash" "*"

"@types/lodash@*":
version "4.14.160"
resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.160.tgz#2f1bba6500bc3cb9a732c6d66a083378fb0b0b29"
integrity sha512-aP03BShJoO+WVndoVj/WNcB/YBPt+CIU1mvaao2GRAHy2yg4pT/XS4XnVHEQBjPJGycWf/9seKEO9vopTJGkvA==

"@types/lodash@^4.14.146":
version "4.14.154"
resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.154.tgz#069e3c703fdb264e67be9e03b20a640bc0198ecc"
Expand Down