980461 - [MMS] Visual adjustments to attachment download indicator

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[José Vittone]

Attachment: sms-discrepancies.png

There are some deviations between the proposed UX and the current implementation.

1. The copy has been reviewed by Tyler, and is not implemented
2. The download button should look like a link
3. While downloading, it should have a different style

Comment 1

[Oleg Zasypkin [:azasypkin]]

It makes sens to fix that only once bug 963013 is landed.

Comment 2

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

Update:

WIP patch, still waiting for bug 963013 to be landed. 

Some concerns:

* Date format in message is different, if patch for bug 963013 isn't going to fix that, we need separate bug for it;
* Do we need separate hover\active styles for download button\link?
* What font is used in visual spec? Looks like it's a bit different than we use in code;
* Spinner should be updated by patch for bug 963013.

Comment 3

[Oleg Zasypkin [:azasypkin]]

Hey Julien, could you please clarify a bit workflow for changing date formats in Gaia apps?

I see that specs in attachment defines new format for 'expiry date'. I'm wondering whether we just need to add new format to [1] and apply it for our particular case. Or it requires more consideration like reviewing all date\time format changes per new visual spec and updating it at once to avoid future format inconsistency bugs?

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/locales/date/date.en-US.properties

Comment 4

[Julien Wajsberg [:julienw]]

I guess the l10n team will know better.

Pike, what do you think? See attachment 8386955 [details] and pay especially attention to the differences in Date format. Do you think it could go to shared as a new l10n property?

Comment 5

[Francesco Lodolo [:flod]]

That would be "%A, %b %d".

Is this same format going to be reused in other apps? It doesn't seem that versatile without explicit year.

Note that other apps (e.g. Calendar) define their own time formats.
https://github.com/mozilla-b2g/gaia/blob/master/apps/calendar/locales/calendar.en-US.properties#L204

Comment 6

[Axel Hecht [:Pike]]

I'd re-ask UX if date formats are inconsistent. Other than that, what :flod said.

Comment 7

[Oleg Zasypkin [:azasypkin]]

Ok, thanks for your replies. I see that nobody uses such format, so it will be probably better for now to define it inside our app, to avoid pollution of shared files. Will confirm format change with our UX though.

Anyway I think that defining new format for any purpose should follow these steps:

1. Check whether required format is defined in shared locale files;
2. If yes - use it;
3. If no - check whether any other apps use the same format internally;
4. If yes - move it to shared and use shared for both apps;
5. If no - define it inside app's locale file.

The same analysis should be performed when we modify\delete existing format. It's "bureaucratic" way, but will help us to keep shared files clean.

Victoria, could you please confirm that format change for expiry date? Do you know whether it is going to be used in other places\apps?

Comment 8

[Victoria Gerchinhoren [:vicky]]

I am not sure about this. Would ask Carrie and Omega to see if they know about a common pattern.

Comment 9

[Carrie Wang [New account: [e-mail]]]

redirect to Omega since he's in charge of Message. Thanks

Comment 10

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Francesco Lodolo [:flod] from comment #5)
> That would be "%A, %b %d".
I think you meant "%A, %b %e".

Actually, my suggestion is "%A, %B %e", same as lock screen, since space is quite enough here.

****

BTW, as I know, we have different time formats in different apps. (That's not good I know.) We tried to let user select date format in Settings, but only for digit-based time format, like 2014/12/31, not for text-based time format, like Dec 31, 2014.
See the spec for more details: https://bug868889.bugzilla.mozilla.org/attachment.cgi?id=8390915

Comment 11

[Oleg Zasypkin [:azasypkin]]

(In reply to Omega Feng [:Omega] from comment #10)
> (In reply to Francesco Lodolo [:flod] from comment #5)
> > That would be "%A, %b %d".
> I think you meant "%A, %b %e".
> 
> Actually, my suggestion is "%A, %B %e", same as lock screen, since space is
> quite enough here.
> 
> ****
> 
> BTW, as I know, we have different time formats in different apps. (That's
> not good I know.) We tried to let user select date format in Settings, but
> only for digit-based time format, like 2014/12/31, not for text-based time
> format, like Dec 31, 2014.
> See the spec for more details:
> https://bug868889.bugzilla.mozilla.org/attachment.cgi?id=8390915

Nice!

Francesco, I see that '%A, %B %e' is used in clock, homescreen, dialer and now sms. Does it make sense to move that format declaration to shared\locales\date now?

Comment 12

[Francesco Lodolo [:flod]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #11)
> Francesco, I see that '%A, %B %e' is used in clock, homescreen, dialer and
> now sms. Does it make sense to move that format declaration to
> shared\locales\date now?

It makes definitely sense (not sure why I wrote "%d" when I was actually thinking of day numbers without decimals).

Comment 13

[Oleg Zasypkin [:azasypkin]]

(In reply to Francesco Lodolo [:flod] from comment #12)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #11)
> > Francesco, I see that '%A, %B %e' is used in clock, homescreen, dialer and
> > now sms. Does it make sense to move that format declaration to
> > shared\locales\date now?
> 
> It makes definitely sense (not sure why I wrote "%d" when I was actually
> thinking of day numbers without decimals).

Ok, filed bug 1001439 to handle this move in a separate patch.

Comment 14

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8406138 [details] [review]
GitHub pull request URL

Hey Julien,

It would be great if you can give feedback on the WIP patch.

One thing that really bothers me is that we have to override too generic styles for buttons inside lists from style_unstable/buttons.css. 

IMO it doesn't look right to define borders and backgrounds with such generic and widely used selector like 'li button, li [role=button]' especially inside shared styles.

Comment 15

[Oleg Zasypkin [:azasypkin]]

Attachment: state screenshots.zip

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8406138 [details] [review]
GitHub pull request URL

looks good, I don't think it's that hacky. Still we can ask Arnau if part of it could go in the BB.

Comment 17

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Comment on attachment 8406138 [details] [review]
> GitHub pull request URL (wip)
> 
> looks good, I don't think it's that hacky. Still we can ask Arnau if part of
> it could go in the BB.

Thanks for feedback!

Arnau, could you please take a look at my PR (especially at ".message .download" css style)? We're wondering whether BB can\will be tweaked to avoid some of these overrides. Thanks!

Comment 18

[Oleg Zasypkin [:azasypkin]]

Attachment: state screenshots (default, pending, error and expired)

Hey Victoria, could you please review screenshots of new download button?

I haven't updated distance between text and download button and border for error bubble. I believe it's going to be updated in bug 963013.

Thanks!

Comment 19

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8414319 [details]
state screenshots (default, pending, error and expired)

Can you please make screenshots from phone instead of emulator? Because I cannot check the syles right... THanks.

Comment 20

[Oleg Zasypkin [:azasypkin]]

Attachment: state screenshots from Buri (default, pending, error and expired)

(In reply to Victoria Gerchinhoren [:vicky] from comment #19)
> Comment on attachment 8414319 [details]
> state screenshots (default, pending, error and expired)
> 
> Can you please make screenshots from phone instead of emulator? Because I
> cannot check the syles right... THanks.

Sure! Here are screenshots from my Buri.

Comment 21

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8415316 [details]
state screenshots from Buri (default, pending, error and expired)

Oleg, please put the text in #333333 as the green bubbles in SMS are now a very light one and when the refresh is ready, the text would not be readable. The rest of styles are ok.

Thanks.

Comment 22

[Oleg Zasypkin [:azasypkin]]

(In reply to Victoria Gerchinhoren [:vicky] from comment #21)
> Comment on attachment 8415316 [details]
> state screenshots from Buri (default, pending, error and expired)
> 
> Oleg, please put the text in #333333 as the green bubbles in SMS are now a
> very light one and when the refresh is ready, the text would not be
> readable. The rest of styles are ok.
> 
> Thanks.

Thanks for review! Updated color and underline color from #fff to #333. As the rest styles are OK, asking for code review then.

Comment 24

[Oleg Zasypkin [:azasypkin]]

Victoria, could you please let us know what text color you'd like to see for :hover/:active state for the attachment download link?

Comment 25

[Julien Wajsberg [:julienw]]

Comment on attachment 8406138 [details] [review]
GitHub pull request URL

Added some comments on github.

Please see avec Victoria to resolve these issues and request review again once you're ready !

Comment 26

[Victoria Gerchinhoren [:vicky]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #24)
> Victoria, could you please let us know what text color you'd like to see for
> :hover/:active state for the attachment download link?

Hi Oleg,

Please use #038282
Thanks!

Comment 27

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8406138 [details] [review]
GitHub pull request URL

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #25)
> Comment on attachment 8406138 [details] [review]
> GitHub pull request URL
> 
> Added some comments on github.
> 
> Please see avec Victoria to resolve these issues and request review again
> once you're ready !

I've updated :active text color as Victoria suggested + removed redundant <form> tag.

Regarding to empty space between donwload button and text, as I mentioned earlier, IMO it would be better and safer to cover it in the scope of bug 963013 as the space is mainly influenced by line-height of p-container that is container not-only for download button. As hot-fix we can reduce line-height for p-container and setting right line-height for span with text only, but I'd rather wait for bubble re-layout for cleaner and generic solution.

What do you think?

Comment 28

[Julien Wajsberg [:julienw]]

I'd like a copy review for the expired attachment text.

Currently it's:

  expired-attachment        = Sorry, message cannot be downloaded as it expired on {{date}}.

I don't like it so much, especially the lack of "the" before "message".

Matej, can you please help here ?

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 8406138 [details] [review]
GitHub pull request URL

r=me with a nit for "type=button" but please wait for the copy review before landing. If the copy review needs to change please change it and carry over my r+.

Also, please ensure a green travis!

Comment 30

[Matej Novak [:matej]]

(In reply to Julien Wajsberg [:julienw] from comment #28)
> I'd like a copy review for the expired attachment text.
> 
> Currently it's:
> 
>   expired-attachment        = Sorry, message cannot be downloaded as it
> expired on {{date}}.
> 
> I don't like it so much, especially the lack of "the" before "message".
> 
> Matej, can you please help here ?

Sure thing. How about:

Sorry, the message expired on {{date}} and cannot be downloaded.

Comment 31

[Julien Wajsberg [:julienw]]

Works for me, thanks !

Comment 32

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #31)
> Works for me, thanks !

Fixed 'type=button' nit and updated copy text as per comment 30.

Travis is green now!

Thanks!

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/f0aed88845112f77eae2e9dc4b24b35a121d33b3

Comment 34

[Francesco Lodolo [:flod]]

One consistency note: for English you should always use "…" (single Unicode character) instead of "..." (downloading-attachment). You can fix that without a new string ID.

Comment 35

[Julien Wajsberg [:julienw]]

Fixed in master: 461ea9a09563c2caf1fe5195227d126dadb83dc5

Thanks Francesco.

Comment 36

[Oleg Zasypkin [:azasypkin]]

Thanks for the note Francesco and Julien for the fix! :)

Comment 37

[Stefan Plewako [:stef]]

With this code it is impossible to use natural language in pl.

For now we will use %d.%m.%Y (ie. 01.01.1970) for expiry-date-format (incompatible with bug 1001439).

If we would like to use natural language comparable to en-US we would need to extend localeFormat function from http://mxr.mozilla.org/gaia/source/shared/js/l10n_date.js#28 with two new grammatical cases (acccusative, genitive) in first letter lower case form (may not be reusable, standalone or sentence start need upper case) for weekday names and define date formatting for not-downloaded-attachment and expired-attachment separately (split expiry-date-format).

Comment 38

[Oleg Zasypkin [:azasypkin]]

(In reply to Stefan Plewako [:stef] from comment #37)
> With this code it is impossible to use natural language in pl.
> 
> For now we will use %d.%m.%Y (ie. 01.01.1970) for expiry-date-format
> (incompatible with bug 1001439).
> 
> If we would like to use natural language comparable to en-US we would need
> to extend localeFormat function from
> http://mxr.mozilla.org/gaia/source/shared/js/l10n_date.js#28 with two new
> grammatical cases (acccusative, genitive) in first letter lower case form
> (may not be reusable, standalone or sentence start need upper case) for
> weekday names and define date formatting for not-downloaded-attachment and
> expired-attachment separately (split expiry-date-format).

Yes, this format (in this particular case) will look ugly for some other languages too, thanks for noticing this! But I'm not sure whether we have common approach to deal with that. 

Francesco, do you know if we had solved similar problems earlier or we just should stick to digits-only formats for such cases?

Comment 39

[Francesco Lodolo [:flod]]

(CCing a few people for the l10n.js part)

(In reply to Oleg Zasypkin [:azasypkin] from comment #38)
> Yes, this format (in this particular case) will look ugly for some other
> languages too, thanks for noticing this! But I'm not sure whether we have
> common approach to deal with that. 

Not really. In the past we had a similar problem with months, and we created a new date format in shared (AFAIK that's the only special case we have)
https://github.com/mozilla-b2g/gaia/blob/master/shared/locales/date/date.en-US.properties#L34

But I don't think that's a solution on the long term (that would be l20n).

> Francesco, do you know if we had solved similar problems earlier or we just
> should stick to digits-only formats for such cases?

English should keep using the current format. But it would probably be good to:
* add a note that this format could not be suitable for locales with more complex grammar rules. 
* add a note for the other two strings explaining that expiry-date-format will be use to format {{ date }}.

Comment 40

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (localization note only)

Added PR with localization notes suggested in comment 39.

Comment 41

[Julien Wajsberg [:julienw]]

Comment on attachment 8422434 [details] [review]
GitHub pull request URL (localization note only)

r=me with adding the suggested replacement for more complex locales

Comment 42

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #41)
> Comment on attachment 8422434 [details] [review]
> GitHub pull request URL (localization note only)
> 
> r=me with adding the suggested replacement for more complex locales

Thanks! Done.

Note to sheriffs: check-in needed for "GitHub pull request URL (localization note only)" PR only.

Comment 43

[Julien Wajsberg [:julienw]]

I don't know whether sheriffs find checkin-needed in RESOLVED/FIXED bugs, so better be safe :)

Comment 44

[Ryan VanderMeulen [:RyanVM]]

We do ;)

Master: https://github.com/mozilla-b2g/gaia/commit/ac4309e47c4e454aed395c78225bc497cc2e9d54