Skip to content
This repository has been archived by the owner on Nov 10, 2021. It is now read-only.

PRIAPI-73: SVG spriting, icons #5

Merged
merged 5 commits into from
Mar 13, 2018
Merged

Conversation

evanmwillhite
Copy link

@evanmwillhite evanmwillhite commented Mar 13, 2018

PRIAPI-73: SVG spriting, icon system

This PR does the following:

  • Adds an SVG spriting and iconography system using https://www.npmjs.com/package/svg-sprite-loader
  • Adds an Icon component with examples in the button and dropdown components
  • Adds an Icon test
  • Tweaks jest settings to ignore *.svg files

To Review:

Questions/Concerns

  • Can you verify that how I ignored SVG files is OK?
  • Does the test look OK and is it enough?
  • Is this line the correct approach for loading the SVG?

@@ -23,6 +24,7 @@ function Button(props) {
className={`${styles[buttonClass]} ${className}`}
onClick={onClick}
>
{icon ? <Icon svg={icon} /> : null}

Choose a reason for hiding this comment

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

What about just { icon && <Icon svg={icon} /> }?

Copy link
Author

Choose a reason for hiding this comment

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

YES

*/
function Icon(props) {
const { svg } = props;
const icon = require(`./svg/${svg}.svg`); // eslint-disable-line

Choose a reason for hiding this comment

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

I would take a look at this

I have no real knowledge of how best to handle this – it's a bit out of my wheel house

/**
* Component that renders a link, or a button with a click handler.
*/
function Icon(props) {
Copy link
Member

Choose a reason for hiding this comment

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

@flipactual correct me if I'm wrong but I think it's safe to use an arrow function here:

const Icon = (props) => {};

Copy link
Member

Choose a reason for hiding this comment

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

Likely should do this in the Button component as well for maximum consistency 💯

import Icon from './Icon.component';

describe('<Icon />', () => {
const icon = 'heart';
Copy link
Member

Choose a reason for hiding this comment

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

No need for this to be a separate variable since it's only being used once.

Copy link
Author

Choose a reason for hiding this comment

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

I think this is because I am telling jest to ignore SVGs. Let's maybe take a stab at this separately.

className={styles.inlineSvg}
fill="currentcolor"
>
<use xlinkHref={`#${icon.default.id}`} />
Copy link
Member

Choose a reason for hiding this comment

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

This is undefined in the snapshot test...

const icon = require(`./svg/${svg}.svg`); // eslint-disable-line
return (
<svg
viewBox={icon.default.viewBox}
Copy link
Member

Choose a reason for hiding this comment

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

This is undefined in the snapshot test...

@@ -23,6 +24,7 @@ function Button(props) {
className={`${styles[buttonClass]} ${className}`}
onClick={onClick}
>
{icon && <Icon svg={icon} />}
Copy link
Member

Choose a reason for hiding this comment

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

Why not just pass the <Icon> directly?

Copy link
Author

Choose a reason for hiding this comment

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

this checks whether the icon exists

}

Icon.propTypes = {
svg: PropTypes.string.isRequired
Copy link
Member

Choose a reason for hiding this comment

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

PropTypes.oneOf(['heart', 'envelope'])

Copy link
Member

@patrickocoffeyo patrickocoffeyo left a comment

Choose a reason for hiding this comment

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

Amazing. Great code, works fantastically! :shipit:

@evanmwillhite evanmwillhite merged commit 28abe4f into develop Mar 13, 2018
@evanmwillhite evanmwillhite deleted the feature/PRIAPI-73-icons branch March 13, 2018 20:24
@pri-bender
Copy link

🎉 This PR is included in version 1.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants