Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v3 #195

Merged
merged 17 commits into from
Oct 1, 2018
Merged

v3 #195

merged 17 commits into from
Oct 1, 2018

Conversation

gregberge
Copy link
Owner

Summary

See commits.

- Add "runtimeConfig" option to enable or disable runtime configuration

BREAKING CHANGE:

- Runtime configuration is always loaded (even with Node API `convert`)
- In CLI, "--config" is now "--config-file"; this new option can be used
everywhere

Closes #192
This will fix all SVGO issues.

BREAKING CHANGE: runtime configuration is now loaded using webpack
loader.

Fixes #177.
Fixes #188.
This will fix all SVGO issues.

BREAKING CHANGE: runtime configuration is now loaded using rollup plugin.

Fixes #177
Fixes #188
@vercel
Copy link

vercel bot commented Sep 30, 2018

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

Removing style tag is an optimization, SVGO can do it better than us.

BREAKING CHANGE:

Style tag will no longer be automatically removed. SVGO should handle it
correctly using "inlineStyles" plugin. If you want to remove them,
enable "removeStyleElement" plugin in your SVGO config.

Closes #191
Also improve template documentation.

Closes #187
@TrySound
Copy link
Contributor

I'm not a fun of Svg prefix. Can this be disabled optionally?

@codecov
Copy link

codecov bot commented Sep 30, 2018

Codecov Report

Merging #195 into master will decrease coverage by 1.14%.
The diff coverage is 82.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
- Coverage   86.88%   85.74%   -1.15%     
==========================================
  Files          26       24       -2     
  Lines         488      519      +31     
  Branches       91      103      +12     
==========================================
+ Hits          424      445      +21     
- Misses         59       66       +7     
- Partials        5        8       +3
Impacted Files Coverage Δ
packages/rollup/src/index.js 100% <ø> (ø) ⬆️
packages/core/src/convert.js 100% <100%> (ø) ⬆️
packages/core/src/templates/reactDomTemplate.js 100% <100%> (ø) ⬆️
packages/core/src/h2x/expandProps.js 100% <100%> (ø) ⬆️
packages/core/src/templates/reactNativeTemplate.js 77.41% <100%> (+0.75%) ⬆️
packages/cli/src/util.js 66.66% <100%> (-4.77%) ⬇️
packages/core/src/plugins/h2x.js 100% <100%> (ø) ⬆️
packages/core/src/config.js 100% <100%> (+22.22%) ⬆️
packages/core/src/plugins/prettier.js 100% <100%> (ø) ⬆️
packages/webpack/src/index.js 95.45% <100%> (ø) ⬆️
... and 10 more

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 b687c9e...a29f45a. Read the comment docs.

@gregberge
Copy link
Owner Author

@TrySound what is the problem with that?

@TrySound
Copy link
Contributor

I use only named exports and wouldn't like to consider icons like something special. I already use icons directory for that. Plain name is enough. Svg prefix is a bit opinionated

@gregberge
Copy link
Owner Author

@TrySound we only use the default export. So I don't see what is the problem. I suggest you to edit your template and remove Svg prefix.

@TrySound
Copy link
Contributor

Okay, if it doesn't affect file name I will just slice prefix.

@gregberge
Copy link
Owner Author

@TrySound yeah it is just the component name to avoid problem like "Infinity =".

@TrySound
Copy link
Contributor

forwardRef implementation is not quite useful. The point is that ref can be builtin option. You suggest to wrap component with React.forwardRef in every usage.

I suggest something like this

import React from 'react';

export default React.forwardRef((props, ref) =>
  <SvgIcon {...props} svgRef={ref} />
)

@gregberge
Copy link
Owner Author

@twm what is the difference between this and my implementation?

@TrySound
Copy link
Contributor

TrySound commented Sep 30, 2018

I don't see you apply React.forwardRef

import React from 'react';

export default (props, ref) =>
  <SvgIcon {...props} svgRef={ref} />

@gregberge
Copy link
Owner Author

gregberge commented Sep 30, 2018

@TrySound https://svgr-mduwqvjvwf.now.sh/

All good except that I forgot React.forwardRef()...

@TrySound
Copy link
Contributor

And? You just name variable forwardRef.
image

@gregberge
Copy link
Owner Author

@TrySound yes, I will name it "ForwardRef" in fact, because it is a component.

@gregberge
Copy link
Owner Author

const ForwardRef = React.forwardRef((props, ref) => ...)
export default ForwardRef

@TrySound
Copy link
Contributor

Yes, this is what library should produce.

BREAKING CHANGE:

"ref" option now uses `React.forwardRef`. You don't have to use "svgRef"
prop, just use "ref" and it will work. `React.forwardRef` requires React
> 16.3.

Closes #184
BREAKING CHANGE: `svgAttributes` has been removed, please use `svgProps` instead.

Closes #173
BREAKING CHANGE:

"--no-expand-props" is now replaced by "--expand-props none". You can now specify a position "start" or "end" for "expandProps"
property.

Closes #170
You can now use `svgr.sync` to call svgr synchronously!

Closes #185
This fix incompatible options, "--no-dimensions" will be prioritary.

Fixes #141
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.

2 participants