1082110 - Polish appearance of new reading list style

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[:Margaret Leibovic]

Follow-up to bug 889351 to refine the style of the new reading list items.

antlam, can you take a look at this when you get back and let me know what you think? At the very least I think we need to tweak the alignment of the estimated reading time, but maybe there are other things you want to refine.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Good call! I'll take a look at this hopefully tomorrow back at the office. Thanks Maragaret!

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

*Margaret :P

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_readinglist_mock1.png

Wanted to post a WIP for this bug. I'm also trying to see what sort of "read/unread" notification we could work in to help address bug 1044089 (those funky dog ears you see on the top right of each list item).

All of this borrows styles from the other panels too so it'd be a nice unifying pass as well. Yay for multiple birds with one bug!

I had a question about the excerpt length, how long (in terms of character count) are we allowing? 

Meanwhile, will knock this out soon so leaving NI on.

Comment 4

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #3)

> I had a question about the excerpt length, how long (in terms of character
> count) are we allowing? 

Right now we limit the excerpt to 4 lines. Looking at the code, I don't see us actually setting a limit on the characters anywhere... we may want to add that, since that would have implications for syncing.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js#740

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_readinglist_mock2.png

Attaching today's WIP. I'm happy with where this is to begin specing out and attaching the specs. Any feedback?

There's two different sizes for these list items since Tablets are typically wider, we could save vertical real estate there. 

I've also opted for the "dog ear" as an indicator for read/unread (bug 1044089).

Also, to design for either long or short headlines, I've center aligned it inside the entire list item, so the only consistency is the margin between the body copy and the title which is 4 dp in this design (and the left and right margins of course).

I think 140 characters is a good limit to set, it's hard to design for a flexible "preview" of the article..

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_readinglist_mock2.png

Attaching a spec of the above design.

Some notes:

 - to design for longer (two line) titles on mobile, I've only defined the padding between the Page title and the body (4dp), I've simply then center aligned the copy vertically.

 - tablet has a shorter height for each item since it is wider, this gives the user a larger, more useful list on Tablets too.
 
 - The height (blue) of the list item does not include the divider since I think that's how it's done (I remember asking Lucas about this) - but just in case, the divider is #D7D9DB

 - I remember determining line-height was finicky, but I simply multiplied the font size by 1.5, neat trick - improves legibility :)

 - For REALLY long page titles, we might have to truncate but I'm not anticipating many will be longer than 2 lines

 - I'll attach the dog ear graphic for read/unread after this

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_dogear.zip

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8509048 [details]
spec_readinglist_mock2.png

Do we not try to show favicons in the RL?  Or bookmark stars?  Or the "Switch to tab"-like thing?

I don't understand why we're making this so different from the other lists in the home panels.  The colors and the spacing are distinct for no particular reason that I can see.  We do have a meaningful read/unread icon, but it doesn't feel the same as the bookmark star.  And the time to read calculation doesn't feel the same as the "Last synced: ..." time on the Synced Tabs panel.

It feels like we're getting away from our existing look and feel and I don't understand what the over-arching unification is.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #8)

> It feels like we're getting away from our existing look and feel and I don't
> understand what the over-arching unification is.

The other panels are limited in the amount of rich content they can display. Reading List has the potential to display much more than a title, url and favicon. Some original designs used a more magazine-like layout, using a primary image from the article as well as the excerpt. I would like to try to patterns that aren't simple lists with favicons.

I agree that common elements should have some common styling, including margins & fonts & colors, but the layout itself should be allowed to evolve beyond the other panels. Looking at the current (and proposed) Reading List, the "reading time" seems like the most different element. The orange is a feature not used in any other lists. I also agree with Nick about trying to keep all of the panels anchored in some common elements, like the switch-to-tab feature.

Comment 10

[:Margaret Leibovic]

(In reply to Nick Alexander :nalexander from comment #8)
> Comment on attachment 8509048 [details]
> spec_readinglist_mock2.png
> 
> Do we not try to show favicons in the RL?

We used to try to show favicons, but I think we want to move towards more of a magazine-like appearance. In the future maybe we can try to get images out of the articles and display them the way we do for dynamic panels.

> Or bookmark stars?

We're not querying the bookmarks db, so we don't actually know whether or not the URLs are bookmarked. But I think reading list should be a different concept from bookmarked URLs.

> Or the "Switch to tab"-like thing?

I think the excerpt is more valuable than switch-to-tab, but this makes me realize we should make sure we're not actually performing switch-to-tab for these items.

> I don't understand why we're making this so different from the other lists
> in the home panels.  The colors and the spacing are distinct for no
> particular reason that I can see.  We do have a meaningful read/unread icon,
> but it doesn't feel the same as the bookmark star.  And the time to read
> calculation doesn't feel the same as the "Last synced: ..." time on the
> Synced Tabs panel.

I agree we should try to be consistent. antlam, have you thought about how this will relate to the other panels? Are you intending to file other bugs about tweaking the existing panels?

I think this design looks like a good place to start, but I think we should make sure the spacing/text sizes are consistent with the other panels. It looks like it might match what we already do, but I could also update the other panels.

As for the read/unread indicator, I think we should just do that as part of bug 1084062, since right now we don't have any way to actually mark an item as read.

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Fair points, Nick. Maybe it was my fault for not giving a complete context to the design decisions made here.

Allow me to elaborate..

I took heavily from the Sync Panel UI since that was the latest file. Font sizes, weights, colors and even the height of these list items were all in some way derived from that file. For example, the height of these items were not arbitrarily designed, they're exactly 2x the height of the Sync Panels' List item (on Mobile) and 1.5 x (on Tablet). Same goes for the clearance space of the "time to read" indicator. The clearance for the favicon on list items on the Sync Panel is 64dp in width and if you compare that to the spec for this Reading list panel, it's the same. Naturally, the height is difference for content length and reasons I mentioned above.

I am trying to unify these awesome panels that we have and that work is being tracked in bug 1063058. I should've added this to the meta bug. That was my fault. :)

(In reply to Nick Alexander :nalexander from comment #8)
> Comment on attachment 8509048 [details]
> spec_readinglist_mock2.png
>
> I don't understand why we're making this so different from the other lists
> in the home panels.  The colors and the spacing are distinct for no
> particular reason that I can see.  

They are not distinct/different. If you look at the specs for the Sync Panel and compare it to this, I use Roboto Light 14sp #363B40 for the headers of both, and Roboto regular, 12sp, #BFBFBF for the subtitle section. These were the values I took from the Sync Panel design I gave you.

> We do have a meaningful read/unread icon,
> but it doesn't feel the same as the bookmark star.  

Fair point, although the 2 icons and actions are not related, I see what you're saying. Maybe I can work on this some more but I thought it at least made a good starting point.

> And the time to read
> calculation doesn't feel the same as the "Last synced: ..." time on the
> Synced Tabs panel.

I'm not sure that it should feel the same. In the context of information hierarchy, for an overarching Sync umbrella, the next useful bit of information to know about Device is "when it was last synced". But in the Reading list panel, I feel like the next most important information for the user after the Article's title would be the opening characters to help them recall in case the title was unclear.

Therefor, being the sort of "cherry on top", the estimated "time to read" has taken the space to the right hand side. This very same space has the same horizontal sizing as the spec'd favicon space in the Sync Panel, I simply moved it to the right.

> It feels like we're getting away from our existing look and feel and I don't
> understand what the over-arching unification is.

Maybe it's because I am trying to unify the panels right now and it's a bit of an ongoing effort. So, as this is a long process, there will be some overlap where some panels feel different to others. But, it's something that we're working towards.

I hope that was helpful!

Comment 12

[:Margaret Leibovic]

Attachment: screenshot

What do you think of this?

Instead of restricting the description to 140 characters, I decided to just limit it to 3 lines, since it looked weird to ellipsize it in the middle of the line if there was more room to show more text. Also I used 1.2 as the line height multiplier, since 1.5 looked like too much space.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8512318 [details]
screenshot

This is looking good! 

How much padding do we have between the title and the excerpt? 

Though 1.2 might be too small, I find the text starts to clutter and feel really busy. Could we try 1.4 and 1.5? :)

Comment 14

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #13)

> How much padding do we have between the title and the excerpt? 

I don't think we specify any. I can add the 4dp in the mock, sorry I missed that.

> Though 1.2 might be too small, I find the text starts to clutter and feel
> really busy. Could we try 1.4 and 1.5? :)

Sure, I can try 1.4. I really think 1.5 was too large (it looked much larger than what was in your mock-up), but I can share screenshots.

Comment 15

[:Margaret Leibovic]

Attachment: Polish appearance of new reading list style

I'm not 100% confident I did the tablet styles correctly, but the idea is that we want shorter rows with only 2 lines of description on all tablets (both portrait and landscape).

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Wanted to leave a note here, 1.35 was the line height multiplier we ended up using as per the empty Private tabs tray that mcomella worked on :)

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8513089 [details] [diff] [review]
Polish appearance of new reading list style

Review of attachment 8513089 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to :Margaret Leibovic from comment #15)
> I'm not 100% confident I did the tablet styles correctly, but the idea is
> that we want shorter rows with only 2 lines of description on all tablets
> (both portrait and landscape).

Does this mean you haven't tested it on tablet yet? Do you mind doing that (and posting a screenshot)?

r+ assuming tablet is roughly to spec and nits.

::: mobile/android/base/home/ReadingListRow.java
@@ +42,5 @@
>          super(context, attrs);
>  
>          LayoutInflater.from(context).inflate(R.layout.reading_list_row_view, this);
>  
> +        setOrientation(LinearLayout.HORIZONTAL);

Why not set this in XML?

::: mobile/android/base/resources/layout/reading_list_row_view.xml
@@ +9,5 @@
> +        android:layout_width="0dip"
> +        android:layout_height="match_parent"
> +        android:layout_weight="1"
> +        android:paddingLeft="15dp"
> +        android:paddingRight="10dp"

The mock shows this should be 15dp on tablet.

::: mobile/android/base/resources/values-large-v11/dimens.xml
@@ +10,5 @@
>  
>      <dimen name="tabs_counter_size">26sp</dimen>
>      <dimen name="panel_grid_view_column_width">200dp</dimen>
>  
> +	<dimen name="reading_list_row_height">96dp</dimen>

nit: indentation.

Comment 18

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #17)
> Comment on attachment 8513089 [details] [diff] [review]
> Polish appearance of new reading list style
> 
> Review of attachment 8513089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to :Margaret Leibovic from comment #15)
> > I'm not 100% confident I did the tablet styles correctly, but the idea is
> > that we want shorter rows with only 2 lines of description on all tablets
> > (both portrait and landscape).
> 
> Does this mean you haven't tested it on tablet yet? Do you mind doing that
> (and posting a screenshot)?

I tested on an N7. Will that cover all the configurations? I guess my uncertainty was based on the fact that I wasn't sure how many different configurations we support.

> ::: mobile/android/base/home/ReadingListRow.java
> @@ +42,5 @@
> >          super(context, attrs);
> >  
> >          LayoutInflater.from(context).inflate(R.layout.reading_list_row_view, this);
> >  
> > +        setOrientation(LinearLayout.HORIZONTAL);
> 
> Why not set this in XML?

I must have been confused and thought that we were creating ReadingListRow instances in Java, but it looks like we do declare this in XML. But actually, we don't even need this because the default is horizontal.

> ::: mobile/android/base/resources/layout/reading_list_row_view.xml
> @@ +9,5 @@
> > +        android:layout_width="0dip"
> > +        android:layout_height="match_parent"
> > +        android:layout_weight="1"
> > +        android:paddingLeft="15dp"
> > +        android:paddingRight="10dp"
> 
> The mock shows this should be 15dp on tablet.

Good catch! Sorry I missed that.

Comment 19

[:Margaret Leibovic]

Attachment: tablet screenshot

Comment 20

[:Margaret Leibovic]

Attachment: phone screenshot

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/c0a39c3bee2e

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0a39c3bee2e

Comment 23

[Teodora Vermesan (:TeoVermesan)]

The limits of the excerpt:
On phone the title is displayed on two lines and the description on 3 lines
On tablet the title is displayed on one line and the description on 2 lines
Verified fixed on:
Devices: Alcatel One Touch(Android 4.1.2) and Nexus 7 (Android 5.0)
Build: Firefox for Android 36.0a1 (2014-11-23)