Skip to content

fix(core): not working first plugin when icon option enabled#137

Merged
gregberge merged 4 commits into
gregberge:masterfrom
plag:master
Jul 11, 2018
Merged

fix(core): not working first plugin when icon option enabled#137
gregberge merged 4 commits into
gregberge:masterfrom
plag:master

Conversation

@plag

@plag plag commented Jul 9, 2018

Copy link
Copy Markdown
Contributor

Hello! Just got not working plugin when enable icon option.

Problem:
not working config:

{
  icon: true,
  plugins: [ { removeDesc: false } ]
}

working config:

{
  icon: true,
  plugins: [ {}, { removeDesc: false } ]
}

Solution: add lodash merge function for concat arrays instead merge them and add test for it.

@plag plag changed the title Fix merge plugins array when icon option enabled fix(core) not working first plugin when icon option enabled Jul 9, 2018
@plag plag changed the title fix(core) not working first plugin when icon option enabled fix(core): not working first plugin when icon option enabled Jul 9, 2018
@codecov

codecov Bot commented Jul 9, 2018

Copy link
Copy Markdown

Codecov Report

Merging #137 into master will decrease coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
- Coverage   88.75%   88.55%   -0.21%     
==========================================
  Files          25       26       +1     
  Lines         418      428      +10     
  Branches      112      100      -12     
==========================================
+ Hits          371      379       +8     
- Misses         34       42       +8     
+ Partials       13        7       -6
Impacted Files Coverage Δ
packages/core/src/plugins/mergeConfigs.js 80% <100%> (ø)
packages/core/src/plugins/svgo.js 100% <100%> (ø) ⬆️
packages/core/src/plugins/h2x.js 96.29% <0%> (ø) ⬆️
packages/core/src/h2x/replaceAttrValues.js 72.72% <0%> (ø) ⬆️
packages/core/src/h2x/svgAttributes.js 76.92% <0%> (ø) ⬆️
packages/core/src/templates/reactNativeTemplate.js 76.66% <0%> (ø) ⬆️
packages/core/src/h2x/removeDimensions.js 83.33% <0%> (ø) ⬆️

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 8fb24b6...6d382d4. Read the comment docs.

@emmenko

emmenko commented Jul 9, 2018

Copy link
Copy Markdown

Ah nice, we bumped into the same problem today as well!

Thanks for the PR, that should fix the problem yes 🙌

One minor suggestion maybe: how about extracting the "config merging" into a separate function and test that? Relying on an SVG snapshot to test merged configs is kind of not intuitive. I/we can also open up a new PR to improve the testing if we don't want to do that in this PR.
WDYT?

@emmenko emmenko left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👌

Comment thread packages/core/src/plugins/svgo.js Outdated


function concatArrays(objValue, srcValue) {
if (isArray(objValue)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nitpick: can't we use Array.isArray?

Comment thread packages/core/src/plugins/svgo.js Outdated
const svgoRcConfig = await explorer.search(filePath)
const svgo = new SVGO(
merge(getBaseSvgoConfig(config), svgoRcConfig, config.svgoConfig),
mergeWith(getBaseSvgoConfig(config), svgoRcConfig, config.svgoConfig, concatArrays),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nitpick: like suggested, I would extract the "merging" into a separate function and test that in isolation to make sure the "merged" config is correct.

@plag

plag commented Jul 9, 2018

Copy link
Copy Markdown
Contributor Author

@emmenko thank you for comments! just make improvements on your suggestions! what about it?

@emmenko

emmenko commented Jul 9, 2018

Copy link
Copy Markdown

Cool, thanks! Much better now 👌
Let's see what the author has to say about it 😇

PS: can you run prettier on the changed/test files? The formatting looks weird.

@plag

plag commented Jul 9, 2018

Copy link
Copy Markdown
Contributor Author

@emmenko sorry for formatting! done with prettier.

@gregberge gregberge merged commit e13a99a into gregberge:master Jul 11, 2018
@gregberge

Copy link
Copy Markdown
Owner

Awesome thanks 🙏 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants