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

Update/add Warning, Info, Check, Cross, TableConfig icons #241

Merged
merged 12 commits into from
May 12, 2016

Conversation

strunkandwhite
Copy link
Contributor

@strunkandwhite strunkandwhite commented May 12, 2016

PR Checklist

  • Manually tested across supported browsers
    • Chrome
    • Firefox
    • Safari
    • IE11 (Win 7)
    • Edge (Win 10)
  • Unit tests written (common at minimum)
  • PR has one of the semver- labels
  • Two core team engineer approvals
  • One core team UX approval

@@ -15,6 +15,12 @@ const InfoIcon = React.createClass({
...Icon.propTypes,
},

getDefaultProps() {
return {
viewBox: '-23.5 0.5 16 16',
Copy link
Contributor

@jondlm jondlm May 12, 2016

Choose a reason for hiding this comment

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

We can leave this out and modify the rects a little bit:

<rect x="7" y="3" width="2" height="2"></rect>
<rect x="7" y="6" width="2" height="7"></rect>

@strunkandwhite strunkandwhite changed the title Update SVGs for Warning and Info icons Update/add Warning, Info, Check, Cross, TableConfig icons May 12, 2016
@strunkandwhite strunkandwhite mentioned this pull request May 12, 2016
9 tasks
…lt props from Chevron, Info, TableConfig, and Warning icons
{...passThroughs}
className={boundClassNames('&', className)}
>
<rect className={boundClassNames('&-background')} x='1' y='4' width='13' height='8'/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation looks funky here

@ogupte
Copy link
Contributor

ogupte commented May 12, 2016

I think the icon name should be more about what it renders than what it might be used for: TableCog or TableGear might be a more appropriate name for this icon. This is more like how other icon libs/sets name their icons.

@strunkandwhite
Copy link
Contributor Author

Makes sense, changing. Also fixing indentation.

@strunkandwhite
Copy link
Contributor Author

Fixes #240, #242, #243

@strunkandwhite strunkandwhite merged commit f9c62d1 into master May 12, 2016
@jondlm jondlm deleted the feature/240-update-icons branch June 8, 2016 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants