Closed Bug 980461 Opened 7 years ago Closed 7 years ago

[MMS] Visual adjustments to attachment download indicator

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.0, tracking-b2g:backlog, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S2 (23may)
feature-b2g 2.0
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: vittone, Assigned: azasypkin)

References

Details

(Whiteboard: [p=1])

Attachments

(4 files, 2 obsolete files)

Attached image 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
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
It makes sens to fix that only once bug 963013 is landed.
Depends on: 963013
Status: ASSIGNED → NEW
Depends on: 994498
Target Milestone: --- → 1.4 S6 (25apr)
Status: NEW → ASSIGNED
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.
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
Flags: needinfo?(felash)
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?
Flags: needinfo?(felash) → needinfo?(l10n)
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
I'd re-ask UX if date formats are inconsistent. Other than that, what :flod said.
Flags: needinfo?(l10n)
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?
Flags: needinfo?(vpg)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
I am not sure about this. Would ask Carrie and Omega to see if they know about a common pattern.
Flags: needinfo?(vpg) → needinfo?(cawang)
redirect to Omega since he's in charge of Message. Thanks
Flags: needinfo?(cawang) → needinfo?(ofeng)
(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
Flags: needinfo?(ofeng)
(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?
Flags: needinfo?(francesco.lodolo)
(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).
Flags: needinfo?(francesco.lodolo)
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
No longer depends on: 963013
Summary: [MMS] Visual adjustments to attachment download indicator & copy is incorrect → [MMS] Visual adjustments to attachment download indicator
(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 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.
Attachment #8406138 - Flags: feedback?(felash)
Attached file state screenshots.zip (obsolete) —
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.
Attachment #8406138 - Flags: feedback?(felash) → feedback+
(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!
Flags: needinfo?(arnau)
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!
Attachment #8412688 - Attachment is obsolete: true
Attachment #8414319 - Flags: ui-review?(vpg)
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.
Attachment #8414319 - Flags: ui-review?(vpg) → ui-review-
(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.
Attachment #8414319 - Attachment is obsolete: true
Attachment #8415316 - Flags: ui-review?(vpg)
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.
Attachment #8415316 - Flags: ui-review?(vpg) → ui-review-
(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.
Attachment #8406138 - Attachment description: GitHub pull request URL (wip) → GitHub pull request URL
Attachment #8406138 - Flags: review?(felash)
Duplicate of this bug: 1006124
Victoria, could you please let us know what text color you'd like to see for :hover/:active state for the attachment download link?
Flags: needinfo?(vpg)
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 !
Attachment #8406138 - Flags: review?(felash)
(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!
Flags: needinfo?(vpg)
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?
Attachment #8406138 - Flags: review?(felash)
blocking-b2g: --- → backlog
Blocks: sms-sprint-1
Whiteboard: [p=1]
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
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 ?
Flags: needinfo?(Mnovak)
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!
Attachment #8406138 - Flags: review?(felash) → review+
(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.
Flags: needinfo?(Mnovak)
Works for me, thanks !
(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!
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/f0aed88845112f77eae2e9dc4b24b35a121d33b3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Fixed in master: 461ea9a09563c2caf1fe5195227d126dadb83dc5

Thanks Francesco.
Thanks for the note Francesco and Julien for the fix! :)
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).
(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?
Flags: needinfo?(francesco.lodolo)
(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 }}.
Flags: needinfo?(francesco.lodolo)
Added PR with localization notes suggested in comment 39.
Attachment #8422434 - Flags: review?(felash)
Comment on attachment 8422434 [details] [review]
GitHub pull request URL (localization note only)

r=me with adding the suggested replacement for more complex locales
Attachment #8422434 - Flags: review?(felash) → review+
(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.
Keywords: checkin-needed
I don't know whether sheriffs find checkin-needed in RESOLVED/FIXED bugs, so better be safe :)
Flags: needinfo?(ryanvm)
Depends on: 1010301
Blocks: 1011558
feature-b2g: --- → 2.0
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.