Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Convert Svg() to JSX#3573

Merged
wldcordeiro merged 1 commit into
firefox-devtools:masterfrom
darkwing:svg-jsx
Aug 5, 2017
Merged

Convert Svg() to JSX#3573
wldcordeiro merged 1 commit into
firefox-devtools:masterfrom
darkwing:svg-jsx

Conversation

@darkwing
Copy link
Copy Markdown
Contributor

@darkwing darkwing commented Aug 4, 2017

Ticking another one off.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 4, 2017

Codecov Report

Merging #3573 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3573      +/-   ##
==========================================
- Coverage   52.59%   52.57%   -0.03%     
==========================================
  Files         119      119              
  Lines        4649     4649              
  Branches      958      958              
==========================================
- Hits         2445     2444       -1     
- Misses       2204     2205       +1
Impacted Files Coverage Δ
assets/images/Svg.js 84.61% <100%> (ø) ⬆️
src/actions/ast.js 76.78% <0%> (-1.79%) ⬇️

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 c5759ea...965c444. Read the comment docs.

@zaggy
Copy link
Copy Markdown
Contributor

zaggy commented Aug 4, 2017

Hi,

Maybe it is worth, while you are at it, to try an update svg-inline-react package to the latest version?

Comment thread assets/images/Svg.js
}
props = Object.assign({}, props, { className, src: svg[name] });
return React.createElement(InlineSVG, props);
return <InlineSVG {...props} />;
Copy link
Copy Markdown
Contributor

@wldcordeiro wldcordeiro Aug 5, 2017

Choose a reason for hiding this comment

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

So this is an idea. But you'd need to touch more of the codebase. How about converting this component from:

module.exports = function(name, props) {
  // eslint-disable-line
  if (!svg[name]) {
    throw new Error("Unknown SVG: " + name);
  }
  let className = name;
  if (props && props.className) {
    className = `${name} ${props.className}`;
  }
  if (name === "subSettings") {
    className = "";
  }
  props = Object.assign({}, props, { className, src: svg[name] });
  return <InlineSVG {...props} />;
};

to

export default function Svg({ name, ...props }) {
  // eslint-disable-line
  if (!svg[name]) {
    throw new Error("Unknown SVG: " + name);
  }
  let className = name;
  if (props && props.className) {
    className = `${name} ${props.className}`;
  }
  if (name === "subSettings") {
    className = "";
  }
  props = { ...props, { className, src: svg[name] } };
  return <InlineSVG {...{ ...props, className, src: svg[name] }} />;
};

We would need to modify the usage of Svg but then we could use it like a component as well and convert the usages like so

// from this
Svg("some-name", { foo: bar });

// to this
<Svg name="some-name", foo={bar} />

@wldcordeiro
Copy link
Copy Markdown
Contributor

@zaggy I would leave it for now as it's a major version bump.

@darkwing
Copy link
Copy Markdown
Contributor Author

darkwing commented Aug 5, 2017

@wldcordeiro I was thinking the same but I didn't want to touch every file that used it yet; easier if we JSX all the components and then make this change

@wldcordeiro
Copy link
Copy Markdown
Contributor

I was thinking the same but I didn't want to touch every file that used it yet; easier if we JSX all the components and then make this change

@darkwing agreed. I'll amend the main issue with that as an item.

@wldcordeiro wldcordeiro mentioned this pull request Aug 5, 2017
41 tasks
@wldcordeiro wldcordeiro merged commit 3bd8840 into firefox-devtools:master Aug 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants