From 88b24c5a3b6e66e5195c0a630a0f1aca63bdf4a0 Mon Sep 17 00:00:00 2001 From: Sudhir Mitharwal <6812992+sudkumar@users.noreply.github.com> Date: Tue, 24 Sep 2019 13:11:59 +0530 Subject: [PATCH] fix(babel-plugin-svg-dynamic-title): dont render empty title (#341) Currently when no title is passed and titleProp is set to `true`, we are rendering an empty title element which is causing some issues. So we have added a conditional statement for rendering the title element only if the title props is not falsy. If we have an existing title, that will rendered by default. Passing the `null` values as the title will cause also cause the title element for not render even if a default title exists on the svg Fixes #333 --- .../src/index.js | 29 +++++++++++++------ .../src/index.test.js | 20 +++++++++---- packages/babel-preset/src/index.test.js | 6 ++-- .../cli/src/__snapshots__/index.test.js.snap | 2 +- .../src/__snapshots__/convert.test.js.snap | 4 +-- 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/packages/babel-plugin-svg-dynamic-title/src/index.js b/packages/babel-plugin-svg-dynamic-title/src/index.js index ae290de1..51f9fb91 100644 --- a/packages/babel-plugin-svg-dynamic-title/src/index.js +++ b/packages/babel-plugin-svg-dynamic-title/src/index.js @@ -11,22 +11,31 @@ const plugin = ({ types: t }) => ({ return } - function createTitle(children = []) { + function createTitle(children = [], attributes = []) { return t.jsxElement( - t.jsxOpeningElement(t.jsxIdentifier('title'), []), + t.jsxOpeningElement(t.jsxIdentifier('title'), attributes), t.jsxClosingElement(t.jsxIdentifier('title')), children, ) } - function getTitleElement(existingTitleChildren = []) { + function getTitleElement(existingTitle) { const titleExpression = t.identifier('title') - let titleElement = createTitle([ - t.jsxExpressionContainer(titleExpression), - ]) - if (existingTitleChildren && existingTitleChildren.length) { + let titleElement = t.conditionalExpression( + titleExpression, + createTitle( + [t.jsxExpressionContainer(titleExpression)], + existingTitle ? existingTitle.openingElement.attributes : [], + ), + t.nullLiteral(), + ) + if ( + existingTitle && + existingTitle.children && + existingTitle.children.length + ) { // if title already exists // render as follows - const fallbackTitleElement = createTitle(existingTitleChildren) + const fallbackTitleElement = existingTitle // {title === undefined ? fallbackTitleElement : titleElement} const conditionalExpressionForTitle = t.conditionalExpression( t.binaryExpression( @@ -38,6 +47,8 @@ const plugin = ({ types: t }) => ({ titleElement, ) titleElement = t.jsxExpressionContainer(conditionalExpressionForTitle) + } else { + titleElement = t.jsxExpressionContainer(titleElement) } return titleElement } @@ -49,7 +60,7 @@ const plugin = ({ types: t }) => ({ if (!childPath.isJSXElement()) return false if (childPath.node === titleElement) return false if (childPath.node.openingElement.name.name !== 'title') return false - titleElement = getTitleElement(childPath.node.children) + titleElement = getTitleElement(childPath.node) childPath.replaceWith(titleElement) return true }) diff --git a/packages/babel-plugin-svg-dynamic-title/src/index.test.js b/packages/babel-plugin-svg-dynamic-title/src/index.test.js index daadde28..18026c0b 100644 --- a/packages/babel-plugin-svg-dynamic-title/src/index.test.js +++ b/packages/babel-plugin-svg-dynamic-title/src/index.test.js @@ -13,36 +13,44 @@ const testPlugin = (code, options) => { describe('plugin', () => { it('should add title attribute if not present', () => { expect(testPlugin('')).toMatchInlineSnapshot( - `"{title};"`, + `"{title ? {title} : null};"`, ) }) it('should add title element and fallback to existing title', () => { // testing when the existing title contains a simple string expect(testPlugin(`Hello`)).toMatchInlineSnapshot( - `"{title === undefined ? Hello : {title}};"`, + `"{title === undefined ? Hello : title ? {title} : null};"`, ) // testing when the existing title contains an JSXExpression expect( testPlugin(`{"Hello"}`), ).toMatchInlineSnapshot( - `"{title === undefined ? {\\"Hello\\"} : {title}};"`, + `"{title === undefined ? {\\"Hello\\"} : title ? {title} : null};"`, + ) + }) + it('should preserve any existing title attributes', () => { + // testing when the existing title contains a simple string + expect( + testPlugin(`Hello`), + ).toMatchInlineSnapshot( + `"{title === undefined ? Hello : title ? {title} : null};"`, ) }) it('should support empty title', () => { expect(testPlugin('')).toMatchInlineSnapshot( - `"{title};"`, + `"{title ? {title} : null};"`, ) }) it('should support self closing title', () => { expect(testPlugin('')).toMatchInlineSnapshot( - `"{title};"`, + `"{title ? {title} : null};"`, ) }) it('should work if an attribute is already present', () => { expect(testPlugin('')).toMatchInlineSnapshot( - `"{title};"`, + `"{title ? {title} : null};"`, ) }) }) diff --git a/packages/babel-preset/src/index.test.js b/packages/babel-preset/src/index.test.js index 81423c4c..eb6196de 100644 --- a/packages/babel-preset/src/index.test.js +++ b/packages/babel-preset/src/index.test.js @@ -63,7 +63,7 @@ describe('preset', () => { const SvgComponent = ({ title - }) => {title}; + }) => {title ? {title} : null}; export default SvgComponent;" `) @@ -82,7 +82,7 @@ describe('preset', () => { const SvgComponent = ({ title - }) => {title === undefined ? Hello : {title}}; + }) => {title === undefined ? Hello : title ? {title} : null}; export default SvgComponent;" `) @@ -99,7 +99,7 @@ describe('preset', () => { const SvgComponent = ({ title - }) => {title === undefined ? {\\"Hello\\"} : {title}}; + }) => {title === undefined ? {\\"Hello\\"} : title ? {title} : null}; export default SvgComponent;" `) diff --git a/packages/cli/src/__snapshots__/index.test.js.snap b/packages/cli/src/__snapshots__/index.test.js.snap index c7aeeeaa..16e7c5bb 100644 --- a/packages/cli/src/__snapshots__/index.test.js.snap +++ b/packages/cli/src/__snapshots__/index.test.js.snap @@ -320,7 +320,7 @@ exports[`cli should support various args: --title-prop 1`] = ` const SvgFile = ({ title, ...props }) => ( - {title} + {title ? {title} : null} ) diff --git a/packages/core/src/__snapshots__/convert.test.js.snap b/packages/core/src/__snapshots__/convert.test.js.snap index 3cef5e7b..e60014fd 100644 --- a/packages/core/src/__snapshots__/convert.test.js.snap +++ b/packages/core/src/__snapshots__/convert.test.js.snap @@ -332,7 +332,7 @@ exports[`convert config should support options { titleProp: true } 1`] = ` const SvgComponent = ({ title, ...props }) => ( - {title} + {title ? {title} : null} ( }} {...props} > - {title} + {title ? {title} : null} )