1062314 - [Utility Tray] Visual Refinements Utility Tray

Status: RESOLVED FIXED

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

Description

[Eric Pang [:epang] UX]

Opening this bug to differentiate the Notification header from the rest of the notifications.

I've updated the spec on box:
https://mozilla.box.com/s/2rjzhlh3vym17b85myxr

Changes include:

1. Header Text
 - 22px
 - Light Italic

2. Dividers
 - Divider under header is full length across screen
 - Divider in between notifications have 1.5rem margins

Comment 1

[Eric Pang [:epang] UX]

Guillaume, can you flag me for ui-review when ready?  Thanks!

Comment 2

[Guillaume Marty [:gmarty]]

WIP in this branch: https://github.com/gmarty/gaia/tree/Bug-1062314-Visual-Refinements-Utility-Tray
As expected, changing the divider is tricky given the current markup. Will take another fresh look later.

Comment 3

[Gregor Wagner [:gwagner]]

Do we still need this bug?

Comment 4

[Eric Pang [:epang] UX]

(In reply to Gregor Wagner [:gwagner] from comment #3)
> Do we still need this bug?

Yes, Guillaume is still working on it :).

Comment 5

[Guillaume Marty [:gmarty]]

Attachment: Github branch

I updated the branch to add the notifications counter in the utility tray.

The behaviour is the same as previously, that is only desktop notifications are counted, others are not (updates, bluetooth, media recording...).

For that reason, Rob, Eric, can you please review and tell me if it looks good to you?

Comment 6

[Rob MacDonald [:robmac]]

Comment on attachment 8490056 [details]
Github branch

There are some discrepancies from the spec in the download scenario (spinner versus a progress bar) but the implementation is a big improvement from an IxD perspective and I wouldn't consider it an issue that would prevent us from landing this at this stage.

Eric will be able to comment on any visual issues.

Comment 7

[Eric Pang [:epang] UX]

Comment on attachment 8490056 [details]
Github branch

Looks good Guillaume, marking as R+ since this bug was mainly to address the header changes and count.  I thought we had the download progress implemented, did we not?  If not it's probably not in scope anymore, but I still this this tray is MUCH better then the one before!

Comment 8

[Guillaume Marty [:gmarty]]

Attachment: Github PR

Thanks guys for the ui-r+. I restyled the progress bar but I didn't replace the loader by a progress bar. This can be done in a follow up bug.

Tim, can you have a look at this patch?
Also, note to self: ping the l10n guys about the change in case.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8490701 [details] [review]
Github PR

Sorry for the really late review -- I was OOO.

I didn't test this locally since you already got ui-review+.

I am not sure if you need to update l10n property here, so I am setting feedback to :l10n.

Comment 10

[Francesco Lodolo [:flod]]

Comment on attachment 8490701 [details] [review]
Github PR

The only change here is letter case, so no need to have new IDs for these strings (it's basically like fixing a typo). Each locale is responsible for using case consistently in their own localization.

:l10n is an alias, it's OK to CC it, not really to assign actionable items to it.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Alias_.3Al10n

Having said that, have someone tested this with a long text for "notification"? Is it going to resize as headers usually do? You can use German for a test ("Benachrichtigungen").

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Francesco Lodolo [:flod] from comment #10)
> :l10n is an alias, it's OK to CC it, not really to assign actionable items
> to it.

Damn, I was meant to needinfo :pike!

Comment 12

[Francesco Lodolo [:flod]]

The slightly difference between l10n@ and :l10n :-)

From a quick test, I'd say this is broken with long text: searched for #notification-some, replaced text with "Benachrichtigungen", I lose the "Delete all" button completely.

Comment 13

[Francesco Lodolo [:flod]]

Guillame, can you take a look before landing this?

I tried to change the string in WebIDE: with a long string the button to clear notifications disappear, with a very long string both title and button disappear.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8490701 [details] [review]
Github PR

We can't land this patch with comment 13.

Thanks :flod.

Comment 15

[Francesco Lodolo [:flod]]

Attachment: Broken UI

This is broken on German, but it's also evident on pseudolocale.

Comment 16

[Eric Pang [:epang] UX]

(In reply to Francesco Lodolo [:flod] from comment #15)
> Created attachment 8493560 [details]
> Broken UI
> 
> This is broken on German, but it's also evident on pseudolocale.

Thanks for the review Francesco.  Let's reduce the font size to fix this.  Guillaume can you update the header text to 18px (27px on flame)?  This should give more then enough space :)

Comment 17

[Guillaume Marty [:gmarty]]

Comment on attachment 8490701 [details] [review]
Github PR

I've updated the patch to:
* Add some flexbox wizardry to get the notification title truncated if it can't fit in
* Address the nits that Tim left on Github
* Reduce the font size as Eric said

Tim, how does it look this time?

Comment 18

[Guillaume Marty [:gmarty]]

Merged in https://github.com/mozilla-b2g/gaia/commit/2d2f9de213b14023f36d33a33cbf28ff1bbe71a4

Comment 19

[Guillaume Marty [:gmarty]]

Comment on attachment 8490701 [details] [review]
Github PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Utility tray visual refresh
[User impact] if declined: UI won't as polish as it should be
[Testing completed]: manual testing
[Risk to taking this patch] (and alternatives if risky): Very low, mainly CSS changes
[String changes made]: none

Comment 20

[Guillaume Marty [:gmarty]]

Ooops, forgot to mention that there is actually a string change: 'statusbarNotifications' must be capitalised.

Comment 21

[[:fabrice] Fabrice Desré]

Comment on attachment 8490701 [details] [review]
Github PR

Not bad enough to justify uplift.

Comment 22

[Guillaume Marty [:gmarty]]

As per Comment 15 above there is a risk of broken layout on languages like German that this patch fixes. Can you please reconsider the decision about uplifting.

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to Guillaume Marty [:gmarty] from comment #22)
> As per Comment 15 above there is a risk of broken layout on languages like
> German that this patch fixes. Can you please reconsider the decision about
> uplifting.

Comment 15 is a screenshot from a version of your PR that was buggy, right? On current trunk I can't reproduce the issue of having "Clear all" to disappear, even with a pseudo locale.

Comment 24

[Guillaume Marty [:gmarty]]

Yes Fabrice, this patch fixes the broken layout with 'clear all' button disappearing on master. However this issue is still present in v2.1 branch. So I'd really like to get this patch uplifted.

Comment 25

[bhavana bajaj [:bajaj]]

(In reply to Guillaume Marty [:gmarty] from comment #24)
> Yes Fabrice, this patch fixes the broken layout with 'clear all' button
> disappearing on master. However this issue is still present in v2.1 branch.
> So I'd really like to get this patch uplifted.

we are past the time to land any string changes on 2.1 at this point, so we cannot uplift the existing patch on 2.1

Comment 26

[[:fabrice] Fabrice Desré]

(In reply to Guillaume Marty [:gmarty] from comment #24)
> Yes Fabrice, this patch fixes the broken layout with 'clear all' button
> disappearing on master. However this issue is still present in v2.1 branch.
> So I'd really like to get this patch uplifted.

I flashed the latests 2.1, and switched to the pseudo language accentuated english. I can't reproduce the issue, likely because the font size is already smaller than in the screenshot in comment 16

Comment 27

[Francesco Lodolo [:flod]]

The only string change in this patch is about capitalization. You could just create a patch specific for 2.1 without string changes at all.

Comment 28

[Stephany Wilkes]

[Blocking Requested - why for this release]: I'm re-noming because there is no string change and this affects a primary shipping market for this release.

If I understand the bug correctly, German translations are affected and this is supposed to ship in Europe.

Why can't this be uplifted? It's not about actual translation; it's about capitalization. The string changes -- as Fabrice points out in comment #27 -- are not needed.

Comment 29

[Gregor Wagner [:gwagner]]

You have to ask for uplift approval. And its fixed on master so please keep it closed.

Comment 30

[Gregor Wagner [:gwagner]]

(In reply to Stephany Wilkes from comment #28)
> [Blocking Requested - why for this release]: I'm re-noming because there is
> no string change and this affects a primary shipping market for this release.
> 
> If I understand the bug correctly, German translations are affected and this
> is supposed to ship in Europe.
> 
> Why can't this be uplifted? It's not about actual translation; it's about
> capitalization. The string changes -- as Fabrice points out in comment #27
> -- are not needed.

Comment 31

[[:fabrice] Fabrice Desré]

Gregor, prove me wrong about what I wrote in comment 26 : I could not repro the issue on 2.1

Comment 32

[Gregor Wagner [:gwagner]]

Attachment: 2014-10-10-11-31-30.png

The use-case with the screenshot works for me as well.