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 some new g2-styled icons to the Icons package. #20284 #21209

Merged
merged 46 commits into from
Apr 21, 2020

Conversation

jameskoster
Copy link
Contributor

Description

Proposing a variety of new icons based on the G2 design language, plus an update to the page icon.

How has this been tested?

Testing is required

Screenshots

icons

The icons I'm proposing are in the right-hand column. Figma link.

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@pablohoneyhoney
Copy link

Nice!

Here some notes:

  • Percent, payment, trending, product looking solid.
  • Shipping, Money, Receipt could use further reduction.
  • Personalize and Institution don't think are working well. Are there other market conventions you've explored?
  • Widgets seems the same as Categories currently.
  • Review could be filled and smaller.
  • Page and Inbox could have a bit more breadth between lines.

@jasmussen
Copy link
Contributor

Thanks for the PR!

It's a little hard to tell from the screenshot — did you adjust the stroke-widths of the adjusted icons? While it isn't a hard rule for any of the new icons, the 1.5px stroke most of them come with ties in very well with the dark outlines of the new block UI. If you can point me to a Figma file, I can help make this thinner stroke width happen.

@jameskoster
Copy link
Contributor Author

jameskoster commented Mar 30, 2020

Personalize and Institution don't think are working well. Are there other market conventions you've explored?

Lots of options we can explore for personalize. Paint brush / tin, etc, or something more abstract.

Institute is tricky. It's inspired by the account_balance icon in Material (which we've been utilising as a bank in our UI), and the majority of other iconographic representations of this term. There are probably some variations I can come up with though.

Widgets seems the same as Categories currently.

Hah, I didn't spot that one. Will pull the widgets proposal.

Review could be filled and smaller.
Page and Inbox could have a bit more breadth between lines.

Will adjust this week.

It's a little hard to tell from the screenshot — did you adjust the stroke-widths of the adjusted icons? While it isn't a hard rule for any of the new icons, the 1.5px stroke most of them come with ties in very well with the dark outlines of the new block UI. If you can point me to a Figma file, I can help make this thinner stroke width happen.

All the strokes are 1.5px. There's a Figma link in the op :)

This reverts commit 1f6211a.
@jasmussen
Copy link
Contributor

All the strokes are 1.5px. There's a Figma link in the op :)

I don't know how I missed that! 🤦‍♂

Thanks!

@jameskoster
Copy link
Contributor Author

jameskoster commented Mar 30, 2020

Here are some revisions / options based on the feedback above. Check the "G2 i2" column:

icons

  • Money is tough. Without the flourishes on the corners I worry the bills look more like keyholes or something. I didn't want to use any currency symbols to avoid potential localisation issues, but if we want to be super clear / simple, that might be something we have to explore. I wonder if we can localise an icon? :D
  • I made the review icon smaller, and filled it. Personally I prefer this outlined rather than filled - I think it is more consistent that way. Happy to go with the consensus though.
  • Institute is tough. Added some options there, but to be honest I prefer the original.

Figma link again if anyone would like to riff on these.

@pablohoneyhoney
Copy link

I like the revisions @jameskoster

Money: would it be simpler as a coin and the dollar sign inside?
Institution: agree let's stick with the original then.
Review: looking good but seems a bit too rounded, could it be less so?
Receipt: I prefer this one.
Screen Shot 2020-03-30 at 1 06 35 PM

Personalize: I wonder if the brush I created in the file could work.
block-brush

@jameskoster
Copy link
Contributor Author

jameskoster commented Mar 31, 2020

Another update:

icons

I think if we call the money icon "Dollar" instead then we can use the currency symbol. That leaves scope to add more currencies in the future.

Made the star slightly less rounded.

Personalize: I wonder if the brush I created in the file could work.

Looks a little like a mop to my eye, but it could work.

Edit: Added some other currency symbols

currency

@pablohoneyhoney
Copy link

Nice, we can retouch the brush for sure.
The currencies look good, maybe smaller within the circle.

@jameskoster
Copy link
Contributor Author

Agreed, and done :)

@mtias mtias added the [Package] Components /packages/components label Apr 14, 2020
@mtias
Copy link
Member

mtias commented Apr 14, 2020

Should dollar, pound, euro be prefixed with "currency-"? Pound in particular would be ambiguous otherwise. I'd also imagine we'd grow it with other currency symbols / localizations.

packages/icons/src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

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

A few notes, but this is looking good.

@jameskoster jameskoster requested a review from jasmussen April 20, 2020 09:59
@jasmussen
Copy link
Contributor

Took a good look at the names of these icons, which is the most important because the vectors are the easiest to change of the lot, and those look good too.

Ship it! All this needs is a rebase. Let me know if you need a hand with that!

@jameskoster
Copy link
Contributor Author

Rebase complete! :)

@jasmussen
Copy link
Contributor

Press that green button and congrats + thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants