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

Updated and added some icons #1374

Merged
merged 6 commits into from
Dec 18, 2018
Merged

Updated and added some icons #1374

merged 6 commits into from
Dec 18, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Dec 14, 2018

The main goal was initially just to add a few more icons, but I also noticed a few that needed some cleanup. By cleanup I mean, tried to align to the pixel grid as much as possible and if the icon couldn't be completely centered, they should all head towards top left.

Fixes #1287

Here are the before and afters of just the ones that were changed (before on the left). I've highlighted the ones of note.

screen shot 2018-12-14 at 01 18 00 am

screen shot 2018-12-14 at 00 15 00 am


And unpin
screen shot 2018-12-14 at 11 07 51 am


@daveyholler I realized looking at the before/afters that you made the pixel alignment alterations in the file, or just hadn't update them in the Sketch library. All I did to yours was move it til it aligned, but I see that your folders stretched taller to align. Let me know if you want the original, taller ones or you're ok with the altered one.


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
  • [ ] This required updates to Framer X components

@cchaos cchaos requested review from snide and ryankeairns December 14, 2018 06:54
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM! Just some questions to satisfy my own curiosity...

  • I think I saw some screenshots in Slack recently, but what's the reason for having the iInCircle and questionInCircle be smaller than other circle-based icons as opposed to setting a different size when you use them? is it because using an 'xs' size, for example, leaves you with a smaller click/hover area?
  • I'm sure there is a good reason for it, but what's the case for having both startEmpty and startEmptySpace if they are the same icon (they look the same in the screenshot at least)?
  • Similar to the variations for the star icon, should we add the same variations for the pin icon?

@cchaos
Copy link
Contributor Author

cchaos commented Dec 14, 2018

what's the reason for having the iInCircle and questionInCircle be smaller than other circle-based icons as opposed to setting a different size when you use them?

Good question. I do think the larger click area is helpful, but I think mainly it's because consumers don't know to resize down these types of icons, so we're just forcing it on them. Though, the iInCircle was always smaller than the full 16x16 so I just made the questionInCircle similar. (Though I guess they're both even a tad smaller than that).

what's the case for having both startEmpty and startEmptySpace

So my thought behind that is if you use them in a toggle button or something where icons switch out, you won't have a moving star.

But that also means that the empty space star if off-center so for most use cases you'd want the star centered, which is why I also left the regular one in there. It's really just for a special use-case, but I'd figured to account for it now rather than have jumpy stars. ;)

should we add the same variations for the pin icon?

I had thought about that one too, but just couldn't think of a option for unpin. If you've got an idea, feel free to through it my way. But I did think just flipping it upside down wasn't obvious. Though maybe we could do a slash through it like the offline symbol.

@ryankeairns
Copy link
Contributor

ryankeairns commented Dec 14, 2018

I knew you'd have good explanations, thanks for writing them out :)

As for the pin, a filled option would be helpful to show the 'on' state. As for the unpin, perhaps we could just rotate it slightly (30deg)? It might look a little odd here, in isolation, but imagine how our current hover state raises the icon... so now it would raise and rotate a little bit to imitate unpinning.

screenshot 2018-12-14 09 47 02

@cchaos
Copy link
Contributor Author

cchaos commented Dec 14, 2018

Haha np.

So we don't really have the idea of swapping out icons on hover, so you probably won't see the "rotation" effect. So we could either do a similar idea to the stars and add (+ / -) next to the pin. Or since you mentioned having a filled version, it seems that just clicking the filled version would then "unpin"?

@ryankeairns
Copy link
Contributor

Yeah, simply adding a filled version of the pin works for me. Thanks!

@cchaos
Copy link
Contributor Author

cchaos commented Dec 14, 2018

Added

@ryankeairns
Copy link
Contributor

Looks good, thanks for adding it!

@cchaos
Copy link
Contributor Author

cchaos commented Dec 17, 2018

@daveyholler Are you ok with the changes to your icons?

@daveyholler
Copy link
Contributor

@cchaos Totally fine with the alterations. 🎉

@cchaos cchaos merged commit fc3c790 into elastic:master Dec 18, 2018
@cchaos cchaos deleted the more-icons branch December 18, 2018 16:57
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