Closed Bug 1195677 Opened 5 years ago Closed 5 years ago

Implement updated design of Gravatar permission request box

Categories

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

defect
Points:
3

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Iteration:
43.1 - Aug 24
Tracking Status
firefox43 --- fixed

People

(Reporter: mikedeboer, Assigned: Mardak)

References

Details

(Whiteboard: [visual refresh][strings])

Attachments

(3 files, 5 obsolete files)

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
Flags: qe-verify+
Flags: firefox-backlog+
Assignee: nobody → edilee
Attached image wip screenshot (obsolete) —
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" ?
Attached patch wip (obsolete) — Splinter Review
What's a good way to test rtl? Setting "intl.uidirection.en-US" to "rtl" doesn't seem to affect the panel contents.
Attached image wip screenshot without dir=rtl (obsolete) —
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.
(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
(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!"
Flags: needinfo?(mmaslaney)
Attached patch v1 (obsolete) — Splinter Review
Switched from > (>) to → (→) and check rtl to use ← (←). Updated string per bug 1196499 and comment 7.
Attachment #8649752 - Attachment is obsolete: true
Attachment #8650306 - Flags: review?(mdeboer)
Whiteboard: [visual refresh] → [visual refresh][strings]
Attached image v1 screenshot (obsolete) —
Attachment #8649751 - Attachment is obsolete: true
Attachment #8649776 - Attachment is obsolete: true
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.
Attachment #8650306 - Flags: review?(mdeboer) → review-
See bug 1183606 for svgo usage hints.
Attached patch v2Splinter Review
(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.
Flags: needinfo?(mmaslaney)
Attachment #8650538 - Flags: review?(mdeboer)
Attached image v2 screenshot
Attachment #8650306 - Attachment is obsolete: true
Attachment #8650307 - Attachment is obsolete: true
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?
(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 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.
Attachment #8650538 - Flags: review?(mdeboer) → review+
Attached image 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.
https://hg.mozilla.org/mozilla-central/rev/c4c4a57e0d74
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Iteration: --- → 43.1 - Aug 24
Dropping qe-verify+ since contacts was removed from hello, see bug 1212079.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.