wordpress-mobile / WordPress-Editor-Android

⛔️ [DEPRECATED] A reusable Android rich text editor component.
GNU General Public License v2.0
188 stars 50 forks source link

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

Closed aforcier closed 8 years ago

aforcier commented 8 years ago

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

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

jasmussen commented 8 years ago

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 commented 8 years ago

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:

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 commented 8 years ago

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

davewhitley commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

aforcier commented 8 years ago

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 commented 8 years ago

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

maxme commented 8 years ago

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 commented 8 years ago

@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 https://github.com/wordpress-mobile/WordPress-Android/pull/3856 to be merged before going the "full vector" way.

aforcier commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

maxme commented 8 years ago
maxme commented 8 years ago

:shipit: