Skip to content

Commit

Permalink
fix(babel-plugin-svg-dynamic-title): dont render empty title (#341)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sudkumar authored and gregberge committed Sep 24, 2019
1 parent 4b4bd2c commit 88b24c5
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 21 deletions.
29 changes: 20 additions & 9 deletions packages/babel-plugin-svg-dynamic-title/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -38,6 +47,8 @@ const plugin = ({ types: t }) => ({
titleElement,
)
titleElement = t.jsxExpressionContainer(conditionalExpressionForTitle)
} else {
titleElement = t.jsxExpressionContainer(titleElement)
}
return titleElement
}
Expand All @@ -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
})
Expand Down
20 changes: 14 additions & 6 deletions packages/babel-plugin-svg-dynamic-title/src/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,36 +13,44 @@ const testPlugin = (code, options) => {
describe('plugin', () => {
it('should add title attribute if not present', () => {
expect(testPlugin('<svg></svg>')).toMatchInlineSnapshot(
`"<svg><title>{title}</title></svg>;"`,
`"<svg>{title ? <title>{title}</title> : null}</svg>;"`,
)
})

it('should add title element and fallback to existing title', () => {
// testing when the existing title contains a simple string
expect(testPlugin(`<svg><title>Hello</title></svg>`)).toMatchInlineSnapshot(
`"<svg>{title === undefined ? <title>Hello</title> : <title>{title}</title>}</svg>;"`,
`"<svg>{title === undefined ? <title>Hello</title> : title ? <title>{title}</title> : null}</svg>;"`,
)
// testing when the existing title contains an JSXExpression
expect(
testPlugin(`<svg><title>{"Hello"}</title></svg>`),
).toMatchInlineSnapshot(
`"<svg>{title === undefined ? <title>{\\"Hello\\"}</title> : <title>{title}</title>}</svg>;"`,
`"<svg>{title === undefined ? <title>{\\"Hello\\"}</title> : title ? <title>{title}</title> : null}</svg>;"`,
)
})
it('should preserve any existing title attributes', () => {
// testing when the existing title contains a simple string
expect(
testPlugin(`<svg><title attr='a'>Hello</title></svg>`),
).toMatchInlineSnapshot(
`"<svg>{title === undefined ? <title attr='a'>Hello</title> : title ? <title attr='a'>{title}</title> : null}</svg>;"`,
)
})
it('should support empty title', () => {
expect(testPlugin('<svg><title></title></svg>')).toMatchInlineSnapshot(
`"<svg><title>{title}</title></svg>;"`,
`"<svg>{title ? <title>{title}</title> : null}</svg>;"`,
)
})
it('should support self closing title', () => {
expect(testPlugin('<svg><title /></svg>')).toMatchInlineSnapshot(
`"<svg><title>{title}</title></svg>;"`,
`"<svg>{title ? <title>{title}</title> : null}</svg>;"`,
)
})

it('should work if an attribute is already present', () => {
expect(testPlugin('<svg><foo /></svg>')).toMatchInlineSnapshot(
`"<svg><title>{title}</title><foo /></svg>;"`,
`"<svg>{title ? <title>{title}</title> : null}<foo /></svg>;"`,
)
})
})
6 changes: 3 additions & 3 deletions packages/babel-preset/src/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe('preset', () => {
const SvgComponent = ({
title
}) => <svg><title>{title}</title></svg>;
}) => <svg>{title ? <title>{title}</title> : null}</svg>;
export default SvgComponent;"
`)
Expand All @@ -82,7 +82,7 @@ describe('preset', () => {
const SvgComponent = ({
title
}) => <svg>{title === undefined ? <title>Hello</title> : <title>{title}</title>}</svg>;
}) => <svg>{title === undefined ? <title>Hello</title> : title ? <title>{title}</title> : null}</svg>;
export default SvgComponent;"
`)
Expand All @@ -99,7 +99,7 @@ describe('preset', () => {
const SvgComponent = ({
title
}) => <svg>{title === undefined ? <title>{\\"Hello\\"}</title> : <title>{title}</title>}</svg>;
}) => <svg>{title === undefined ? <title>{\\"Hello\\"}</title> : title ? <title>{title}</title> : null}</svg>;
export default SvgComponent;"
`)
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/__snapshots__/index.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ exports[`cli should support various args: --title-prop 1`] = `
const SvgFile = ({ title, ...props }) => (
<svg width={48} height={1} {...props}>
<title>{title}</title>
{title ? <title>{title}</title> : null}
<path d=\\"M0 0h48v1H0z\\" fill=\\"#063855\\" fillRule=\\"evenodd\\" />
</svg>
)
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/__snapshots__/convert.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ exports[`convert config should support options { titleProp: true } 1`] = `
const SvgComponent = ({ title, ...props }) => (
<svg width={88} height={88} {...props}>
<title>{title}</title>
{title ? <title>{title}</title> : null}
<g
stroke=\\"#063855\\"
strokeWidth={2}
Expand Down Expand Up @@ -361,7 +361,7 @@ const SvgComponent = ({ title, ...props }) => (
}}
{...props}
>
<title>{title}</title>
{title ? <title>{title}</title> : null}
<path d=\\"M0 0h24v24H0z\\" fill=\\"none\\" />
</svg>
)
Expand Down

0 comments on commit 88b24c5

Please sign in to comment.