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

Added new OutwardArrowsIcon #725

Merged
merged 1 commit into from
Feb 27, 2017
Merged

Added new OutwardArrowsIcon #725

merged 1 commit into from
Feb 27, 2017

Conversation

jondlm
Copy link
Contributor

@jondlm jondlm commented Feb 27, 2017

Fixes #708

http://docspot.devnxs.net/projects/lucid/708-icons/#/components/OutwardArrowsIcon

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

@jondlm jondlm self-assigned this Feb 27, 2017
@@ -49,7 +49,6 @@ import CrossIcon from './components/Icon/CrossIcon/CrossIcon';
import DangerIcon from './components/Icon/DangerIcon/DangerIcon';
import DangerLightIcon from './components/Icon/DangerLightIcon/DangerLightIcon';
import DataTable from './components/DataTable/DataTable';
import EmptyStateWrapper from './components/EmptyStateWrapper/EmptyStateWrapper';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just sorting alphabetically


const cx = lucidClassNames.bind('&-OutwardArrowsIcon');

const paths = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a single icon that supports all three rotations

Copy link
Contributor

Choose a reason for hiding this comment

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

will there never be a use case for diagonal the other way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, it could be diagonal-inverted 🚎

But yeah, I don't think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

should be oneOf(['horizontal', 'vertical', '/', '\'])

Copy link
Contributor

@mute mute left a comment

Choose a reason for hiding this comment

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

🆒


const cx = lucidClassNames.bind('&-OutwardArrowsIcon');

const paths = {
Copy link
Contributor

Choose a reason for hiding this comment

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

will there never be a use case for diagonal the other way?

@codecov-io
Copy link

Codecov Report

Merging #725 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #725      +/-   ##
==========================================
+ Coverage   91.65%   91.67%   +0.02%     
==========================================
  Files         161      162       +1     
  Lines        2803     2810       +7     
  Branches      791      791              
==========================================
+ Hits         2569     2576       +7     
  Misses        223      223              
  Partials       11       11
Impacted Files Coverage Δ
...nents/Icon/OutwardArrowsIcon/OutwardArrowsIcon.jsx 100% <100%> (ø)

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 9d57103...b2a17a0. Read the comment docs.

@jondlm jondlm merged commit 7c9eb91 into master Feb 27, 2017
@jondlm jondlm deleted the 708-icons branch February 27, 2017 22:35
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.

4 participants