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

Use defaultProps instead of adding props to the svg #7

Closed
lencioni opened this issue Mar 20, 2017 · 3 comments
Closed

Use defaultProps instead of adding props to the svg #7

lencioni opened this issue Mar 20, 2017 · 3 comments

Comments

@lencioni
Copy link
Contributor

Currently, the transformed component ends up looking something like this:

_extends({
  xmlns: 'http://www.w3.org/2000/svg',
  viewBox: '0 0 1000 1000'
}, props),

this adds the overhead of _extends. I think it might be a better to instead move these values into defaultProps on the generated component.

@kesne
Copy link
Collaborator

kesne commented Mar 20, 2017

I'm not sure how that'd work out... we don't know the props ahead of time, so we can't statically set defaultProps.

Here's an example of how this all shakes out:

import MySvg from './file.svg';
const Test = () => <MySvg prop="red" />;

This turns into:

const MySvg = (props) => <svg {...props} />;
const Test = () => <MySvg prop="red" />;

Feel free to re-open if I'm mistaken.

@kesne kesne closed this as completed Mar 20, 2017
@lencioni
Copy link
Contributor Author

The case I'm talking about here is if the <svg> element in the SVG file itself has attributes. This plugin currently transforms it into something like this:

<svg foo="bar"></svg>
import MySvg from './file.svg';

into:

const MySvg = (props) => <svg foo="bar" {...props} />;

What I am suggesting is to instead convert these static props that are in the SVG file to defaultProps instead, like this:

const MySvg = (props) => <svg {...props} />;
MySvg.defaultProps = {
  foo: "bar",
};

@kesne
Copy link
Collaborator

kesne commented Mar 20, 2017

Ah okay, that makes more sense. I can look into it, and I'd accept a PR that does that. Although I'm not super convinced that Object.assign is going to be slow enough where it matters a whole lot.

@kesne kesne reopened this Mar 20, 2017
lencioni added a commit to lencioni/babel-plugin-inline-react-svg that referenced this issue Mar 20, 2017
Currently, when transforming an SVG file that has attributes on the
top-level `<svg>` element, the transformed component ends up looking
something like this:

```js
_extends({
  xmlns: 'http://www.w3.org/2000/svg',
  viewBox: '0 0 1000 1000'
}, props),
```

this adds the overhead of _extends. I think it might be a better to
instead move these values into `defaultProps` on the generated
component.

Closes airbnb#7
@kesne kesne closed this as completed in #8 Sep 26, 2017
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

No branches or pull requests

2 participants