Skip to content
This repository was archived by the owner on Nov 12, 2020. It is now read-only.

Issue #370: Use gridicons instead of dashicons for the format bar - #375

Merged
maxme merged 9 commits into
developfrom
issue/370-dashicons-to-gridicons
May 19, 2016
Merged

Issue #370: Use gridicons instead of dashicons for the format bar#375
maxme merged 9 commits into
developfrom
issue/370-dashicons-to-gridicons

Conversation

@aforcier

@aforcier aforcier commented May 12, 2016

Copy link
Copy Markdown
Contributor

Fixes #370, replacing the format bar's dashicons with gridicons, and also eliminating the density-specific PNGs in favor of a single VectorDrawable for each icon.

TODO

- [x] Finalize icons - [x] [Update support library to 23.4](https://github.com/wordpress-mobile/WordPress-Android/pull/3856) and make sure everything still works - [x] Check default, 360dp, 380dp, and 600dp (tablet) layout buckets (default may require spacing adjustments to the chevron with the change to the link icon) - [x] Check all API levels to make sure PNG generation is working correctly

Not complete yet - the padding on the final result isn't quite right, the icons look a bit larger than they're supposed to (@jasmussen and I are looking into it):

format-bar-gridicons-380dp-bucket

cc @jasmussen

aforcier added 3 commits May 7, 2016 17:06
- Dropped layer-list state drawables as they don't currently seem to support VectorDrawables
- Removed unneeded android:gravity="center" from selectors, this was also causing issues with VectorDrawables
- Deleted all PNG format bar images and added VectorDrawables in drawable/
@aforcier aforcier added this to the 1.1 milestone May 12, 2016
@jasmussen

Copy link
Copy Markdown

Just to help investigations, here's a GIF animation comparing dashicon buttons to gridicon buttons:

comparison

For reference, Dashicons use a 20px grid, Gridicons use a 24px grid. But both would be scaled up and have padding added, so any inconsistencies in sizing between the two, could be due to different optical sizing — i.e. perhaps the icon shape is bigger on the base canvas on Gridicons than on Dashicons. Here's a comparison to unearth that:

bold-comparison

comparison-still

Note, that's on a 48px canvas, so the 20px dashicon is upsized to 48, which makes it slightly blurry. But nonetheless it looks in both of the comparisons above, that the optical sizing is on point.

Yet when looking at the bar as a whole the dashicons still look a bit large:

barcomparisonstill

Specifically the media item. Here's a comparison of the optical size between those:

mediacomparison

Yep. My initial math is definitely off.

@jasmussen

Copy link
Copy Markdown

Or maybe it wasn't off. It looks like there is no official dashicon "add media" icon at all. It may be one we've downsized and created.

When I compare the old PNGs with the SVGs that were used as bases for the new drawables, I get this:

gif-comparison

comparison-svg-vs-old-png

Looking at this, it seems like the math was right after all, but what threw us off was a couple of things:

  • There was no "add media" icon in dashicons, so my comparison was faulty.
  • Some icons, like Bold and Italic shared optical sizing between dashicons and gridicons.
  • Other icons, like ul and ol, were bigger, optically sized, in gridicons than in dashicons, making the comparisons look off.

So it seems we've done everything right — the icons in gridicons are just bigger in their optical sizing.

Which means we have two options going forward with this PR:

  1. Accept the bigger icons
  2. Downsize the icons, but keep the button size.

I think I'll ask some other designers for opinions. Personally I'm leaning towards downsizing them a teensy bit.

@aforcier

Copy link
Copy Markdown
Contributor Author

FWIW, I also think downsizing them a bit would look better.

@davewhitley

Copy link
Copy Markdown

I agree that they look too big.

Since Gridicons was designed on the same grid/size as the Material Design icons, can we just let them be their native size (24dp)? I wouldn't try to match the size of the old icons.

@jasmussen

Copy link
Copy Markdown

I'll give that a shot, thanks Dave. (though I suspect we'll want some padding around even the base size regardless).

No matter what we end up doing, I have this running in my Android Studio now, and the concensus seems to be "smaller icons than they are in the screenshots". Should be doable, and keeping them crisp.

@jasmussen

Copy link
Copy Markdown

Alright I think I have it. Dave's tip of using the base 24dp size instead of the 36dp size the dashicons used, seems to look good. Here's a mockup I started with:

mockup

May look a bit small at first glance, but here's Google Docs for reference:

ref

Exactly 1:1 size. So for a 44dp button size, where previously icons were 36dp with 2x4dp padding, the new buttons have 24dp icons with 2x10dp padding.

@aforcier here's a zip with the new icons:

exports, iteration 3.zip

@aforcier

aforcier commented May 17, 2016

Copy link
Copy Markdown
Contributor Author

Thanks @jasmussen, I converted those to VectorDrawables and plugged them in:

format-bar-gridicons-v3-44dp-height

I have a feeling something strange is going on with spacing around SVGs, those look a bit squashed to me. I tried increasing the format bar height from 44dp to 46dp and I think it looks better:

format-bar-gridicons-v3-46dp-height

What do you think?

(On tablets we use a 49dp height format bar so it shouldn't be a problem there, but I'll be testing all screen sizes before we merge this to be sure.)

@aforcier

Copy link
Copy Markdown
Contributor Author

Also the HTML text might be a little bit too small now. I tried reverting just the HTML icon to the last version you gave me, it looks like this:

format-bar-gridicons-v3-46dp-height-large-html

@jasmussen

Copy link
Copy Markdown

Yep, the icons are definitely being squashed, the media icon is supposed to be a perfect square. I'm not sure what's going on, the svgs looked right to me. I'll try and run the branch, see if anything jumps out. Plan b, provide you with PNGs instead, the Dashicons weren't squashed.

@aforcier

Copy link
Copy Markdown
Contributor Author

I was able to fix the squashing issue. The 380dp bucket (basically applies to recent high-end phones in portrait mode) uses layout_weight to evenly spread out the icons, and for some reason this wasn't working well with VectorDrawables. They were being stretched out horizontally instead of having spacing added between them, even with strict layout_width/layout_height values. I fixed that by wrapping each icon in a LinearLayout which handles the layout_weight and centers the icon inside it.

Here's the fixed version for 380dp:

format-bar-gridicons-v3-fixed

How's that @jasmussen? Also what do you think about the HTML icon? Here's another version of the above with the previous padding for the HTML icon:

format-bar-gridicons-v3-fixed-large-html

@jasmussen

Copy link
Copy Markdown

VAST improvement, thanks for working on this.

However the buttons look a little bit smaller now than before. I made this comparison to illustrate:

example

The top editor bar there, is from the mockup I made previously, where I worked with 44dp buttons with a 24dp icon. Looks like the new icons have the right aspect ratio and padding, but are downsized from what they should be. They appear to be closer to 37dp than 44dp. Otherwise they look great.

For comparison again, the Google Docs editor uses 24dp icons, and their editor bar is 48dp tall:

https://cloud.githubusercontent.com/assets/1204802/15284692/2fec0c36-1b53-11e6-83fb-f5eae1fbc20f.png

@aforcier

Copy link
Copy Markdown
Contributor Author

I took a look thinking the format bar wasn't being rendered at the proper height, but I don't think that's what's happening. I looked at a screenshot from my 5X (all the ones I've been using), which isn't a 3.0 density screen but actually a 2.6 one. 2.6 x 44 = 114, which is about the height I'm measuring in the screenshot. I took another screenshot using a Nexus 5 emulator, which does have a 3.0 density screen:

format-bar-gridicons-nexus5

I measure the height to be ~132px, as expected. Is this the cause or am I missing something?

@maxme

maxme commented May 18, 2016

Copy link
Copy Markdown
Contributor

IMO, buttons look too small (too much padding).

@aforcier can you take a screenshot (or push the latest changes) with "Show Layout Bounds" enabled in Dev Settings https://cloudup.com/cq7YA9Qas0F - so we can see if padding comes from the linear layout or the drawable.

@jasmussen

Copy link
Copy Markdown

In this screenshot, the buttons look perfect to me. Neither too big nor too small. They also feel like "true Android", which is full of 24dp buttons.

I think the direct 1:1 comparison should be Google Docs. In my opinion the buttons should ideally be exactly the same size (48dp), and icons should be 24dp. I've used Google Docs for a long time, and the buttons there feel right to me, in size and icon size. If the icons feel too small (as @maxme suggests), perhaps that's due to the very light color — perhaps the base color should be darker? Here's a mockup that uses #87a6bc gray:

format-bar-button-size-mockup

I don't know what Google Docs does with the format bar size across device sizes, but I do think whatever they do should be the gold standard for what we do.

@aforcier

Copy link
Copy Markdown
Contributor Author

@maxme: here are the latest gridicons with 'Show Layout Bounds' enabled:

format-bar-gridicons-fixed-layout-bounds

The buttons are naturally smaller per @jasmussen's earlier comment, here's a comparison of the original dashicon on the left and the new gridicon (as generated from the XML) on the right:

ol-dashicon-vs-gridicon

@aforcier

Copy link
Copy Markdown
Contributor Author

Re: Google Docs, here's a screenshot on my 5X:

google-docs-format-bar-nexus-5x

I measure the height of the format bar at 124px, which for a 2.6 density screen makes it ~48dp, not 44dp, on the Nexus 5X.

If it's 44dp on the Nexus 5, perhaps they're bucketing by density.

@jasmussen

Copy link
Copy Markdown

That looks great to me @aforcier ! It seems my previous comparison of was based on the wrong assumption that the 5x was a pure 3x resolution device.

The icon sizes also seem good to me. Like I said previously, if the icons appear visually too small, we should darken the icons.

@aforcier

Copy link
Copy Markdown
Contributor Author

Sorry @jasmussen, looks like I misread your comment and you actually are measuring the Google Docs format bar to be 48dp on your device as well, so that explains everything. I also missed that the 5X was not pure 3x originally.

@aforcier

Copy link
Copy Markdown
Contributor Author

Updated with the latest icons and the 380dp bucket stretching fix for the format bar icons. Also bumped the support libraries to 23.4.0.

@aforcier

Copy link
Copy Markdown
Contributor Author

I'll give the darker color a try on the 5X and see how that looks.

@aforcier

Copy link
Copy Markdown
Contributor Author

Here's the 5X with #87a6bc gray icons:

device-2016-05-18-084124

Looks better to me, though still a bit small - however I'm very used to the previous icons by this point and don't really trust my judgement. cc @jasmussen @maxme

@jasmussen

Copy link
Copy Markdown

Provided their size works consistently across devices, that last screenshot looks super duper to me. Many thumbs up from me.

@maxme

maxme commented May 18, 2016

Copy link
Copy Markdown
Contributor

however I'm very used to the previous icons by this point and don't really trust my judgement.

Yes that's probably why they feel too small to me as well.

@maxme

maxme commented May 18, 2016

Copy link
Copy Markdown
Contributor

@aforcier I'm not seeing vectorDrawables.useSupportLibrary = true in the build.gradle file, that means the current version will generate pngs at build time. I'm not sure if you're waiting for wordpress-mobile/WordPress-Android#3856 to be merged before going the "full vector" way.

@aforcier

Copy link
Copy Markdown
Contributor Author
  • Changed format bar icon color to #87A6BC grey
  • Fixed a stretching issue on 600dp+ screens
  • Tested on all our sizing buckets (default, 360dp, 380dp and 600dp)
  • Tested at each API level from 16 to 24 (preview) to confirm generated PNGs are working, including a physical API16 Nexus S and an API18 Samsung S3

Nexus 9 portrait, N preview (768dp):

370-format-bar-nexus9-600dp

Note that the format bar is 49dp rather than 44dp on tablets, though the icons remain at 44dp:

370-format-bar-nexus9-600dp-bounds

Nexus S portrait, API23 (320dp, scrollable format bar with chevron):

370-format-bar-nexuss-320dp-1

370-format-bar-nexuss-320dp-2

The Nexus 5X and Nexus 5 screenshots from earlier represent the 380dp and the 360dp sizing buckets respectively.

cc @jasmussen for a design once-over.

@jasmussen

Copy link
Copy Markdown

I think it looks good! Here's a screenshot I took from my Nexus 7 of Google Docs, and size-relationship-wise I feel like they are in the same space.

docs

@aforcier

Copy link
Copy Markdown
Contributor Author

Thanks @jasmussen! Looks like this is ready for review @maxme.

@maxme

maxme commented May 19, 2016

Copy link
Copy Markdown
Contributor
  • Looks good on all supported layout buckets https://cloudup.com/c5vMrHwRDgl
  • PNG generated in wpandroid even with vectorDrawables.useSupportLibrary = true in the wpandroid project

@maxme

maxme commented May 19, 2016

Copy link
Copy Markdown
Contributor

:shipit:

@maxme
maxme merged commit 08e4067 into develop May 19, 2016
@maxme
maxme deleted the issue/370-dashicons-to-gridicons branch May 19, 2016 11:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants