fix: text rendering with invalid x or y

Hare-Lucius · Aug 26, 2020 · c7225f0 · c7225f0
1 parent b860240
commit c7225f0
Show file tree

Hide file tree

Showing 6 changed files with 124 additions and 59 deletions.
diff --git a/packages/vx-text/package.json b/packages/vx-text/package.json
@@ -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",
+    "lodash.memoize": "^4.1.2",
     "reduce-css-calc": "^1.3.0"
   },
   "peerDependencies": {

diff --git a/packages/vx-text/src/Text.tsx b/packages/vx-text/src/Text.tsx
@@ -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;
@@ -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;
@@ -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} />;
+    }
+
     let startDy: string | undefined;
     if (verticalAnchor === 'start') {
       startDy = reduceCSSCalc(`calc(${capHeight})`);
@@ -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;
@@ -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}>

diff --git a/packages/vx-text/src/util/getStringWidth.ts b/packages/vx-text/src/util/getStringWidth.ts
@@ -1,4 +1,4 @@
-import memoize from 'lodash/memoize';
+import memoize from 'lodash.memoize';
 
 const MEASUREMENT_ELEMENT_ID = '__react_svg_text_measurement_id';
 

diff --git a/packages/vx-text/test/Text.test.tsx b/packages/vx-text/test/Text.test.tsx
@@ -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', () => {
@@ -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();
   });
@@ -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');
+  });
 });
diff --git a/packages/vx-text/test/svgMock.ts b/packages/vx-text/test/svgMock.ts
@@ -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() {
+  // @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;
+}
diff --git a/yarn.lock b/yarn.lock
@@ -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"