Skip to content
This repository has been archived by the owner on Jun 18, 2020. It is now read-only.

Update Varya Icons #52

Merged
merged 20 commits into from
May 20, 2020
Merged

Update Varya Icons #52

merged 20 commits into from
May 20, 2020

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Apr 6, 2020

Addresses #41

This PR makes an attempt to clean up the icons bundled with Varya. Historically, Varia used Material icons, however the GPL compatibility of that icon set is continually undecided. In addition, the Material icons aren't quite as delicate as the typography in this new default style.

To avoid any potential license issues, and to get the aesthetic synced up a bit more, this PR seeks to replace the existing icons with new ones. To start with, I've pulled in the new G2 iconset that's bundled with Gutenberg. These have a thinner wight, and actually feel relatively decent alongside our body/UI typeface, Fira Sans. A few additional notes:

  • The original design comps used icons sparingly, but the ones it used were generally from Material's "Outlined" set. As noted above, I think we should avoid these due to potential license issues.
  • The icons we use here should match nicely with the theme, but also be generic enough that they'll fit nicely with a range of additional themes that use Varya as a base. Ideally, each theme doesn't need to create its own full set of icons each time. (This is why the theme used Material originally... those icons fit in a lot of different contexts).
  • We can of course draw our own brand new iconset, but I'd rather not reinvent the wheel if we don't actually need to.

Some questions:

  • Is it weird to use the icons from Gutenberg on the front end? cc @pablohoneyhoney + @jasmussen for some perspective here.
  • There were a couple icons that did not exist in the new set, so I built them: Shopping Cart and User. Do these look right?

Screenshots:

Icons (Outlined)

In Context:

Before:

Screen Shot 2020-04-06 at 11 29 40 AM

Screen Shot 2020-04-06 at 11 29 47 AM

After:

Screen Shot 2020-04-06 at 11 10 44 AM

Screen Shot 2020-04-06 at 11 11 02 AM

@kjellr kjellr added the enhancement New feature or request label Apr 6, 2020
@kjellr kjellr requested review from allancole, melchoyce and jffng April 6, 2020 15:47
@kjellr kjellr self-assigned this Apr 6, 2020
@melchoyce
Copy link

Looks really good in context.

Super minor, but I think the cart would look a little more balanced with the right wheel moved in 1px:

image

@jasmussen
Copy link
Member

I'm not quite able to tell the difference in that icon suggestion, but you should feel super free to suggest tweaks in WordPress/gutenberg#20284!

Cool work!

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

🚢. We should avoid using material icons, and I think these icons fit better with the theme.

@jasmussen
Copy link
Member

I think it's okay to use material icons. Most of the time they count as images which aren't necessarily subject to the same license terms as the theme itself, because "the theme can function without them". But I agree it's fuzzy, and the g2 icons are nice so a win either way 😅

@kjellr
Copy link
Contributor Author

kjellr commented Apr 8, 2020

Yeah, I think we just want to avoid any potential issues. And these icons generally work well with the theme anyway.

In any case, I think I might need to re-hint a few of these before merge... They're looking just a little fuzzy at the sizes we need:

Screen Shot 2020-04-08 at 10 03 37 AM

@jasmussen
Copy link
Member

Oh you're sizing them down? To what size? I'd love to get them to work well at 18px.

@kjellr
Copy link
Contributor Author

kjellr commented Apr 8, 2020

They're currently 16px in our comps, which should work pretty well — when these are at 24px it appeared that many strokes are 1.5px. I think the issue is just that some of these lines sit just a pixel higher or lower than they'd need to in order to get the lines to be crisp.

@jasmussen
Copy link
Member

Ah interesting. Yes I suppose 1.5px should render as 1. I somehow doubt we can find line positions that consistently makes it possible to collapse to 1px crisp lines without either size taking a hit. But I'd be happy to be proven wrong.

@pablohoneyhoney any thoughts?

Copy link
Collaborator

@allancole allancole left a comment

Choose a reason for hiding this comment

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

Yeah, I think this look great and if we can avoid GPL issues—even better! I also really like the icon name/id improvements 👍

LGTM!

@allancole
Copy link
Collaborator

I think this is good to merge @kjellr 👍
(unless we’re waiting for more design feedback from others)

@kjellr
Copy link
Contributor Author

kjellr commented Apr 9, 2020

The icons are still looking a little fuzzy for me at this size, so let's hold off. I'll try cleaning these up a little bit soon.

@kjellr kjellr marked this pull request as draft April 10, 2020 13:31
@allancole
Copy link
Collaborator

allancole commented Apr 14, 2020

@kjellr can we add a download icon to this list? Thinking it might be useful for the file download button in WooCommerce, and on the media/attachment page (where you can download media/files directly).

@kjellr kjellr added this to the Varya: Initial Release milestone Apr 16, 2020
jffng added a commit that referenced this pull request May 14, 2020
*Removes various unnecessary filters and functions leftover from Twenty Nineteen and _s.

*Unify all instances of main-menu and main-navigation to use primary- prefix for consistency across name spaces.

*Add download icon to media attachment page (requires #52)

*Remove custom social-menu wrapper class.

*Dequeue Gutenberg theme styles in favor of our own.

*Reset border style on hr and separator block

*Cleans up responsive rules to better support block alignments across different blocks.
@kjellr
Copy link
Contributor Author

kjellr commented May 20, 2020

This should be ready for a final review. I took the default WP icons and pixel-hinted them so they appear nice and crisp at the 16px and 24px sizes we need to use them for. The revisions should be subtle (except for the smaller cart icon, which I created myself and then realized was not properly sized relative to the other icons in the set). 🙂

Icons (Outlined)

Screen Shot 2020-05-20 at 9 34 35 AM
Screen Shot 2020-05-20 at 9 34 45 AM

Menu Closed Menu Open
Screen Shot 2020-05-20 at 9 22 53 AM Screen Shot 2020-05-20 at 9 22 43 AM

@kjellr kjellr marked this pull request as ready for review May 20, 2020 13:36
@allancole
Copy link
Collaborator

This is looking excellent @kjellr! Much cleaner and lighter than what we had before. Couple things to consider before merging.

It’d be nice to have a file download icon in this set. I’m thinking we could use it on the File Download Block, in the media.php/image.php templates where users can download a file and in WooCommerce downloadable products. Not an total necessity but it’d be nice to have.

Secondly, WooCommerce has its own icons that we should probably replace with these semi-custom ones. Theres quite a few of them and some of them are easier to change than others, so maybe its best for this to happen in a separate PR:

Mini-cart Cart Address Alerts Image Zoom
image image image image image image

@kjellr
Copy link
Contributor Author

kjellr commented May 20, 2020

It’d be nice to have a file download icon in this set. I’m thinking we could use it on the File Download Block, in the media.php/image.php templates where users can download a file and in WooCommerce downloadable products. Not an total necessity but it’d be nice to have.

f38fe7d adds a download icon, based on the upload icon in Gutenberg.

Screen Shot 2020-05-20 at 12 51 15 PM

I think we can leave implementation of this icon out of this PR.

Secondly, WooCommerce has its own icons that we should probably replace with these semi-custom ones. Theres quite a few of them and some of them are easier to change than others, so maybe its best for this to happen in a separate PR:

Hmm interesting. But yeah, I think that should be a separate, self-contained PR.

@jffng
Copy link
Contributor

jffng commented May 20, 2020

Agreed, these look much shaper and lighter. Thanks for taking this on @kjellr !

@jffng
Copy link
Contributor

jffng commented May 20, 2020

I can open up some PRs to incorporate the download icon once this is merged.

@kjellr
Copy link
Contributor Author

kjellr commented May 20, 2020

Awesome, thanks everyone!

@kjellr kjellr merged commit 3a34bb5 into master May 20, 2020
@kjellr kjellr deleted the update/varya-icons branch May 20, 2020 18:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request [Theme] Varya
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants