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

Add lockOpoen icon #1215

Merged
merged 3 commits into from
Sep 27, 2018
Merged

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Sep 26, 2018

Summary

Fixes #1214
Adds open version of the existing lock icon.

screenshot 2018-09-26 15 45 31

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Looks good. You'll need to add the typescript definition as well.

https://github.com/elastic/eui/blob/master/src/components/icon/index.d.ts

@cchaos
Copy link
Contributor

cchaos commented Sep 27, 2018

Thanks for taking the initiative on this. I'm a little worried that it's not different enough from the locked icon. It's only missing 4px of a line.

Of course the obvious answer is reflect the top area horizontally, but we don't have room for that. My other (bad) idea is to rotate it slightly, but it's a semi-circle so that won't really make a difference but make the empty space larger.

¯\_(ツ)_/¯

I'll leave it up to you if you want to tweak further or just go with this.

@ryankeairns
Copy link
Contributor Author

@cchaos That was basically my thought process and then I ended up back at this point.

If we made the box part of the icon thinner, then we could flip the hook part open. I’ll whip that up and share it here to see if that feels better.

Thanks for the feedback!

@ryankeairns
Copy link
Contributor Author

ryankeairns commented Sep 27, 2018

@cchaos how does this look to you? It's only two pixels less wide on the square piece:

screenshot 2018-09-27 08 11 44

screenshot 2018-09-27 08 30 58

@cchaos
Copy link
Contributor

cchaos commented Sep 27, 2018

If the hook was a bit wider I think it would be ok to also scoot the unlocked hook a little left to fit it?

@ryankeairns
Copy link
Contributor Author

@cchaos I've updated my previous comment so we can see both versions next to one another.

The latest version feels most clear where the hook has more space between it and the box. The wider hook also looks more like the original version, so current users of the lock icon likely won't notice the change, for what that's worth.

@cchaos
Copy link
Contributor

cchaos commented Sep 27, 2018

Oh, yeah, that last one looks great!

@cchaos
Copy link
Contributor

cchaos commented Sep 27, 2018

I even think I like the skinnier lock better anyway.

@ryankeairns
Copy link
Contributor Author

Cool, yeah, I like the skinnier version as well. Thanks for taking a look!

@ryankeairns ryankeairns merged commit 65ae158 into elastic:master Sep 27, 2018
@snide snide mentioned this pull request Oct 3, 2018
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

Successfully merging this pull request may close these issues.

3 participants