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

Allow to pass dynamic svg props #165

Closed
wants to merge 3 commits into from
Closed

Allow to pass dynamic svg props #165

wants to merge 3 commits into from

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Sep 7, 2018

In this diff I introduce the new option svgProps which is similar to
svgAttributes but applies values in interpolation style {true}.

This allows to specify expressions depending on template values for
example default currentColor in fill prop.

I mentioned in comment about later fixing api to smooth-code/h2x#13

@TrySound
Copy link
Contributor Author

TrySound commented Sep 7, 2018

With this svgAttributes can be removed later in major release

@codecov
Copy link

codecov bot commented Sep 7, 2018

Codecov Report

Merging #165 into master will decrease coverage by 1.04%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   88.91%   87.86%   -1.05%     
==========================================
  Files          25       27       +2     
  Lines         424      478      +54     
  Branches       78       87       +9     
==========================================
+ Hits          377      420      +43     
- Misses         45       55      +10     
- Partials        2        3       +1
Impacted Files Coverage Δ
packages/core/src/config.js 100% <ø> (ø) ⬆️
packages/core/src/plugins/h2x.js 100% <100%> (ø) ⬆️
packages/core/src/h2x/svgAttributes.js 76.92% <100%> (ø) ⬆️
packages/core/src/h2x/svgProps.js 77.5% <77.5%> (ø)
packages/core/src/util.js 100% <0%> (ø) ⬆️
packages/core/src/plugins/mergeConfigs.js 80% <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 62847ec...d3f93e3. Read the comment docs.

@gregberge
Copy link
Owner

Adding svgProps is confusing because users will not understand the difference between svgProps and svgAttributes. Also svgProps is only for maybe 1% of users.

I suggest to modify svgAttributes with this behaviour:

svgAttributes: {
  bool: true, // bool={true}
  numeric: 1, // numeric={1}
  string: 'string', // string="string"
  dynamic: '`dynamic`', // dynamic={dynamic}
}

This way, it will be easy to add dynamic values even in CLI:

svgr --svg-attributes dynamic="`dynamic`"

What do you think?

@TrySound
Copy link
Contributor Author

TrySound commented Sep 8, 2018

I don't suggest to keep both in the final api. svgProps is a clear name about what is expected. Attributes is html/xml term and its is always a string. I suggest to add this option for minor release and remove svgAttributes in the next major one.

@TrySound
Copy link
Contributor Author

TrySound commented Sep 8, 2018

Ah, I see what you suggest. I'm not sure about using `` for expressions. Also types detection from strings in cli may lead to inconsistencies with config.

In this diff I introduce the new option `svgProps` which is similar to
svgAttributes but applies values in interpolation style `{true}`.

This allows to specify expressions depending on template values for
example default `currentColor` in `fill` prop.

I mentioned in comment about later fixing api to smooth-code/h2x#13
@gregberge
Copy link
Owner

If we keep only svgProps, how to specify a svg attribute in command line?

Using command line it will result to this:

  • svgr --svg-props foo="bar" => <svg foo={bar}>
  • svgr --svg-props foo=bar => <svg foo={bar}>

My problem is that your use-case is minimal, people expects:

  • svgr --svg-props foo="bar" => <svg foo="bar">
  • svgr --svg-props foo=bar => <svg foo="bar">

I think we have to keep the two options.

@gregberge
Copy link
Owner

If backquote notation is a problem, we could imagine:

svgr --svg-props foo={bar}

What do you think?

@TrySound
Copy link
Contributor Author

Won't this case be interpolated by bash somehow?

@TrySound
Copy link
Contributor Author

TrySound commented Sep 15, 2018

Okay, this case is good for me --svg-props "foo={'bar'}"

@TrySound
Copy link
Contributor Author

@neoziro Done

@gregberge
Copy link
Owner

See #172

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.

None yet

2 participants