Handle the existing title case by using element instead of value (children)

[sudkumar]

Summary

Fixes #314 titleProps=true not working as expected for existing title

We need to handle existing title element's children with a different strategy. Instead of looping through children and creating a children, we can instead, use all the children and push them into a title element. And from the conditional statement for fallback title, instead of returning a title children, we return the title.

- {title === undefined ? "Existing_Children" : title}
+ {title === undefined ? <title>{Existing_Children}</title> : <title>{title}</title>}

Test plan

I have updated the test case for the same with different type of existing title elements.

// pass
expect(testPlugin(`<svg><title>Hi</title></svg`)). toMatchInlineSnapshot(
`<svg>{title === undefined ? <title>Hi</title> : <title>{title}</title>}</svg>`
)
// pass
expect(testPlugin(`<svg><title>{"Hi"}</title></svg`)). toMatchInlineSnapshot(
`<svg>{title === undefined ? <title>{"Hi"}</title> : <title>{title}</title>}</svg>`
)

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

[gregberge]

Thanks for the update, can you avoid using forEach in tests, it is kind of messy. I would rather prefer just isolated if with duplication. Is it OK for you?

[sudkumar]

Yes. I realised the same. Test should be testable while reading :). Will update them.

[sudkumar]

@neoziro I updated the tests.

There is one more thing I want to do here. I want to preserve the attributes of the existing title.

I just need to update the createTitle function to accept the attributes and we should be good to go.

- 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,
     )
}

And in the getTitleElement

- function getTitleElement(existingTitleChlidren) {
+ function getTitleElement(existingTitle) {
        const titleExpression = t.identifier('title')
        let titleElement = createTitle(
          [t.jsxExpressionContainer(titleExpression)],
+         existingTitle ? existingTitle.openingElement.attributes : [],
        )
-      if (existingTitleChildren && existingTitleChildren.length)
+      if (existingTitle && existingTitle.children &&  existingTitle.children.length) {

-        const fallbackTitleElement = createTitle(existingTitle.children)
+        const fallbackTitleElement = existingTitle

And in our loop over path.get('children'), we can

-       titleElement = getTitleElement(childPath.node.children)
+       titleElement = getTitleElement(childPath.node)

[codecov]

Codecov Report

Merging #315 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
+ Coverage   86.58%   86.63%   +0.05%     
==========================================
  Files          30       30              
  Lines         477      479       +2     
  Branches      135      134       -1     
==========================================
+ Hits          413      415       +2     
  Misses         53       53              
  Partials       11       11

Impacted Files	Coverage Δ
...ckages/babel-plugin-svg-dynamic-title/src/index.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79069ed...7172429. Read the comment docs.

[codecov]

Codecov Report

Merging #315 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
+ Coverage   86.58%   86.63%   +0.05%     
==========================================
  Files          30       30              
  Lines         477      479       +2     
  Branches      135      134       -1     
==========================================
+ Hits          413      415       +2     
  Misses         53       53              
  Partials       11       11

Impacted Files	Coverage Δ
...ckages/babel-plugin-svg-dynamic-title/src/index.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79069ed...7172429. Read the comment docs.

[gregberge]

It looks good to me. I merge it.

[sudkumar]

Hi @neoziro . Any updates on release that includes this PR ?

[gregberge]

Sorry I have just published it!