1195677 - Implement updated design of Gravatar permission request box

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mike de Boer [:mikedeboer]]

You will find the updated spec at https://www.dropbox.com/sh/t1sqeqsnxcc0ze7/AACHXziG0s4HJ7c5gC_euCwEa?dl=0 (Dropbox folder)

And the fox avatar (SVG) as shown in the spec can be found at https://bugzilla.mozilla.org/attachment.cgi?id=8643116

Comment 1

[Mike de Boer [:mikedeboer]]

Dropbox link to the actual spec file: https://www.dropbox.com/sh/t1sqeqsnxcc0ze7/AACHXziG0s4HJ7c5gC_euCwEa?dl=0&preview=Hello_FTUContacts_specs.png

Comment 2

[Ed Lee :Mardak]

Attachment: wip screenshot

What sizes should the various text/buttons be? What color should the ">" between avatars be?

Should the text for "start importing or adding ones" be ".. adding some" ?

Comment 3

[Ed Lee :Mardak]

Attachment: wip

Comment 4

[Ed Lee :Mardak]

What's a good way to test rtl? Setting "intl.uidirection.en-US" to "rtl" doesn't seem to affect the panel contents.

Comment 5

[Ed Lee :Mardak]

Attachment: wip screenshot without dir=rtl

Oops. Adding this rule was undoing some other flipping

- html[dir="rtl"] .contacts-gravatar-avatars {
-  transform: rotateY(180deg);

Attached is without this rule. Although with this rule, the firefox avatar faces left but arrow points the wrong way.

Comment 6

[Andrei Oprea [:andreio]]

(In reply to Ed Lee :Mardak from comment #4)
> What's a good way to test rtl? Setting "intl.uidirection.en-US" to "rtl"
> doesn't seem to affect the panel contents.

https://github.com/mozilla/gecko-dev/blob/ba30ae4dbe8a06477235dc322844b4381d1541b2/browser/components/loop/modules/MozLoopService.jsm#L1503-L1510

Comment 7

[Sevaan Franks [:sevaan]]

(In reply to Ed Lee :Mardak from comment #2)
> Created attachment 8649751 [details]
> wip screenshot

Side-by-side with Mockup: http://i.sevaan.com/image/1P13301S0f2D
 
> What sizes should the various text/buttons be? What color should the ">"
> between avatars be?

Text: 12pt
Buttons: w120px x h30px
>: #9B9B9B

Hmm, not sure if we should be using a > anyway....that could read as "No Gravatars are greater than Gravatars". Maybe it should be an arrow.

NI'ing Mmaslaney for his input/asset-if-necessary

> Should the text for "start importing or adding ones" be ".. adding some" ?

This string will be changed to "No contacts yet. Add someone!"

Comment 8

[Ed Lee :Mardak]

Attachment: v1

Switched from > (>) to → (→) and check rtl to use ← (←). Updated string per bug 1196499 and comment 7.

Comment 9

[Ed Lee :Mardak]

Attachment: v1 screenshot

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8650306 [details] [diff] [review]
v1

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

Getting close!

This patch has bittrotted, likely mostly due to bug 1184559.

::: browser/components/loop/content/css/contacts.css
@@ +289,5 @@
>    border-radius: 2px;
> +  background-color: #fff;
> +  font-size: 1.2rem;
> +  margin: 5px 15px;
> +  padding: 15px 10px;

I think it'd make sense to change the padding to `rem` units.

@@ +340,5 @@
> +  vertical-align: middle;
> +  width: 50px;
> +}
> +
> +/* Adjust the Firefox avatar because it has pointy ears */

nit: missing dot.

@@ +342,5 @@
> +}
> +
> +/* Adjust the Firefox avatar because it has pointy ears */
> +.contacts-gravatar-avatars img:last-child {
> +  transform: scale(1.08) translateY(-2px);

Nice! \o/

::: browser/components/loop/content/css/panel.css
@@ +281,5 @@
>    background-image: url("../shared/img/empty_contacts.svg");
>    background-repeat: no-repeat;
>    background-position: top center;
>    padding-top: 19%;
> +  padding-bottom: 5%;

This already looked rather arbitrary with 3% :-P Oh wel...

@@ +297,2 @@
>  .contact-list-empty {
>    padding-top: 27%;

...so I guess this needs to change to 25%?

::: browser/components/loop/content/js/contacts.jsx
@@ +135,5 @@
>            <p dangerouslySetInnerHTML={{__html: message}}
>               onClick={this.handleLinkClick}></p>
> +          <div className="contacts-gravatar-avatars">
> +            <img src="loop/shared/img/avatars.svg#orange-avatar" />
> +            {mozL10n.getDirection() === "rtl" ? "←" : "→"}

I'd still like to have an SVG asset for the arrow (which we can transform with CSS for rtl). I'm wary of the different appearance it will have with different fonts that are used on the different platforms (OSX, win, lin).

::: browser/components/loop/content/shared/img/firefox-avatar.svg
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

Please use the svgo tool (`npm install -g svgo`) to compress this file better. It's simply to large right now.

Comment 11

[Mike de Boer [:mikedeboer]]

See bug 1183606 for svgo usage hints.

Comment 12

[Ed Lee :Mardak]

Attachment: v2

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> ::: browser/components/loop/content/css/contacts.css
> > +  margin: 5px 15px;
> > +  padding: 15px 10px;
> I think it'd make sense to change the padding to `rem` units.
Done. Updated various margin/paddings/sizes to rem.

> > +/* Adjust the Firefox avatar because it has pointy ears */

> nit: missing dot.
Done.

> ::: browser/components/loop/content/css/panel.css
> > +  padding-bottom: 5%;
> This already looked rather arbitrary with 3% :-P Oh wel...
I left this untouched now that I rebased on the other contacts changes.

> ::: browser/components/loop/content/js/contacts.jsx
> > +            {mozL10n.getDirection() === "rtl" ? "←" : "→"}
> I'd still like to have an SVG asset for the arrow
No SVG needed for 2 lines! I made it with border + transform ;)

> ::: browser/components/loop/content/shared/img/firefox-avatar.svg
> > +<?xml version="1.0" encoding="utf-8"?>
> Please use the svgo tool
Done.

Comment 13

[Ed Lee :Mardak]

Attachment: v2 screenshot

Comment 14

[Ed Lee :Mardak]

Comment on attachment 8650538 [details] [diff] [review]
v2

>+++ b/browser/components/loop/content/js/contacts.jsx
>+            <Button additionalClass="secondary"
>+                    caption={mozL10n.get("gravatars_promo_button_nothanks2")}
>                     onClick={this.handleCloseButtonClick}/>
Should I shift caption/onClick to 2 spaces instead of aligning with additionalClass as well as putting a space before "/>" while we're touching one of the lines?

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to Ed Lee :Mardak from comment #14)
> Comment on attachment 8650538 [details] [diff] [review]
> v2
> 
> >+++ b/browser/components/loop/content/js/contacts.jsx
> >+            <Button additionalClass="secondary"
> >+                    caption={mozL10n.get("gravatars_promo_button_nothanks2")}
> >                     onClick={this.handleCloseButtonClick}/>
> Should I shift caption/onClick to 2 spaces instead of aligning with
> additionalClass as well as putting a space before "/>" while we're touching
> one of the lines?

You should, if you please,
1) just indent `caption` and `onClick` with two spaces, nothing more,
2) put a space between the end of the last attribute and `/>`.

Thanks!

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8650538 [details] [diff] [review]
v2

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

r=me with the two comments addressed. Thanks!

::: browser/components/loop/content/css/contacts.css
@@ +414,5 @@
> +}
> +
> +/* Adjust the Firefox avatar because it has pointy ears. */
> +.contacts-gravatar-avatars img:last-child {
> +  transform: scale(1.08) translateY(-2px);

It looks better for me when you make this 1.15. What do you think?

@@ +425,5 @@
> +  display: inline-block;
> +  height: 1.5rem;
> +  transform: rotateZ(45deg);
> +  vertical-align: middle;
> +  width: 1.5rem;

Now I see what you did here... neat!

When you fix the horizontal alignment, it'll be perfect.

Comment 17

[Ed Lee :Mardak]

Attachment: 1.08 vs 1.15

I used 1.08 because it matches the size of the other avatar's circle. But we could make it larger if that's what we want.

Comment 18

[Pulsebot]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[Bogdan Maris, Desktop Test Engineering]

Dropping qe-verify+ since contacts was removed from hello, see bug 1212079.