1184924 - Implement the refreshed design for the invitation overlay

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

To meet the acceptance criteria as stated in bug 1179164, we should:

 - Implement the new style sharing buttons. Do not add the buttons that we don't have an implementation for (yet).
 - Implement a new context menu (information popover) that informs the user that the link was copied. This link will fade out and hide after 2 seconds.
 - Make sure that the conversation toolbar is visible on top of the overlay.

For the visual design spec, please check out the ones attached to bug 1179164.

Comment 1

[Mike de Boer [:mikedeboer]]

Sevaan, what should we do with 'Add some context' link that is visible when you open a conversation without context attached? (It's the one with the pencil icon in front of it.)

Comment 2

[Sevaan Franks [:sevaan]]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Sevaan, what should we do with 'Add some context' link that is visible when
> you open a conversation without context attached? (It's the one with the
> pencil icon in front of it.)

That link has been moved to the gear menu: http://i.sevaan.com/image/393d2T103U2F

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Sevaan Franks [:sevaan] from comment #2)
> That link has been moved to the gear menu:
> http://i.sevaan.com/image/393d2T103U2F

Fair enough, I thought perhaps that might not be prominent enough...

Comment 4

[Sevaan Franks [:sevaan]]

Visibility is not really a concern for the moment. In the near future context won't work the way it does not, with just being a URL attached to the room. As we move forward in web-sharing, adding context is just bringing a page into your web sharing window, and we are already exploring mechanisms to do that.

Comment 5

[Romain Testard [:RT]]

Just for clarification, the "Submit Feedback" option in the gear menu on http://i.sevaan.com/image/393d2T103U2F is not required.

Comment 6

[Ed Lee :Mardak]

Bug 1178304 will add the facebook button. The buttons shown on the invite view:

(share on Facebook) (share with contacts) (copy link) (?unlabeled email link)

What should happen when clicking on the contacts button "share with contacts" that seems to show a arrowpanel with contacts and their email addresses. Does this include all contacts?

These round buttons have a static blue #00a9dc and hover light blue #5cccee and an active-for-10-seconds green #56b397 each with a white glyph/icon.

Comment 7

[Ed Lee :Mardak]

ni? for the images for the buttons:

https://www.dropbox.com/sh/46pyq5wwgnif6g8/AACEqjLwDw1be0oMYw-okHA9a?dl=0&preview=Share_Screen.png

Comment 8

[Sevaan Franks [:sevaan]]

Michael,

Would you be able to provide these assets? http://i.sevaan.com/image/2w2i0C3S2x42

The Facebook icon can be found here: https://www.facebookbrand.com/ But the rest are just fontawesome glyphs so I suspect they can't be used.

Comment 9

[Ed Lee :Mardak]

Separate from icons, there's the new "share with contacts" button and arrowpanel. Should that be added in this bug as it's new functionality that doesn't seem to be defined? In particular what should appear in the arrowpanel and what should happen when clicking on the button?

Comment 10

[Sevaan Franks [:sevaan]]

No, no need to implement that. It appears Contacts will be going away in forthcoming releases.

Comment 11

[Ed Lee :Mardak]

Attachment: wip screenshot

How opaque should the overlay be? It used to be 60%. I changed it to 90% for this screenshot.

What should the text be for hovering over email and after clicking? I assumed "email link" and "emailed!"

What should happen if the user clicks on one button causing the text to appear for a few seconds but then hovers over the other button causing two sets of button text to appear? Potentially they could overlap depending on how close we place the buttons. (But in the screenshot, both happen to be spaced apart enough.)

On a related note how should the buttons be spaced? Right now they're just equally spaced (flex).

(I've put some placeholder images for the button for now.)

Comment 12

[Ed Lee :Mardak]

Attachment: wip

We can land a string-only patch by taking the .properties file and only including the additions (don't remove to-be-unused strings as they're still used).

Comment 13

[Ed Lee :Mardak]

Attachment: added strings patch

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8663085 [details] [diff] [review]
added strings patch

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

rs=dmose

Comment 15

[Pulsebot]

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

Comment 16

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/9d8166cf45a2bc6b1c5d1dab4dc6dc37214dad0d
Bug 1184924 - Implement the refreshed design for the invitation overlay strings [rs=dmose]

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Francesco Lodolo [:flod]]

Where can I see specs/visual to understand how strings are used? I'm quite surprised by the lowercase.

As a side note, I don't really understand why we host specs on external services instead of using Bugzilla's attachments, at least for low-res versions. The direct result of this decision is that the link provided in comment 7 is broken, and that happens a lot in Loop bugs.

Comment 19

[Sevaan Franks [:sevaan]]

Attachment: Invitation Screens for Conversation Window

(In reply to Ed Lee :Mardak from comment #11)
> Created attachment 8663052 [details]
> wip screenshot
> 
> How opaque should the overlay be? It used to be 60%. I changed it to 90% for
> this screenshot.t 

Let's do it at 85%, thanks!
 
> What should the text be for hovering over email and after clicking? I
> assumed "email link" and "emailed!"

I don't think it needs to change. There is a change of state behind the scenes with copying to a clipboard. But when clicking the email button it's obvious what the change is, and I can do it multiple times. I don't think we need the "Shared!" either for sharing with Facebook now that I'm looking at the spec.
 
> What should happen if the user clicks on one button causing the text to
> appear for a few seconds but then hovers over the other button causing two
> sets of button text to appear? Potentially they could overlap depending on
> how close we place the buttons. (But in the screenshot, both happen to be
> spaced apart enough.)

We should instantly revert back, so the action text goes back to it's original state instantly.
 
> On a related note how should the buttons be spaced? Right now they're just
> equally spaced (flex).

All buttons should be centered with 8px gap between them.

(In reply to Francesco Lodolo [:flod] from comment #18)
> Where can I see specs/visual to understand how strings are used? I'm quite
> surprised by the lowercase.

I agree here. Can we have the first letter uppercase, please?

I'll attach a more up-to-date spec to this bug to address dead linkage with Dropbox.

Comment 20

[Ed Lee :Mardak]

Attachment: old invite spec

Here's the design I've been working with from the now-dead link.

If we do want to make the first letter uppercase, we'll need to get it in really soon/today.

Comment 21

[Francesco Lodolo [:flod]]

(In reply to Ed Lee :Mardak from comment #20)
> If we do want to make the first letter uppercase, we'll need to get it in
> really soon/today.

I think we're already past that, merges are happening as we speak, so this will go into fx44.

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/9d8166cf45a2

Comment 23

[Ed Lee :Mardak]

Attachment: wip v2 video

Ignoring the icon for now.. clicking on the button and waiting 2 seconds for the triggered state to go away.. then clicking again then hovering over the other button.

Comment 24

[Ed Lee :Mardak]

Attachment: wip v2

Comment 25

[Sevaan Franks [:sevaan]]

Attachment: Invitation Assets (SVG)

Assets from :mmaslaney attached.

Comment 26

[Ed Lee :Mardak]

Attachment: svg icons filled with background fills only the icon

(In reply to Sevaan Franks [:sevaan] from comment #25)
> Created attachment 8664198 [details]
> Invitation Assets (SVG)
> Assets from :mmaslaney attached.
These should be inverted the other way. There should be white for the icon and transparent for the background (and no need for the circle). E.g., just a white mail icon with no/transparent background.

Comment 27

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Hello_Assets_092015-v2.zip

New assets attached

Comment 28

[Ed Lee :Mardak]

Attachment: v1 screenshot

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #27)
> Created attachment 8664377 [details]
> Hello_Assets_092015-v2.zip
> New assets attached
Thanks! Looks good!

Comment 29

[Ed Lee :Mardak]

Attachment: v1

Comment 30

[Mark Banner (:standard8)]

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

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

Looks good. r=Standard8 with the comments addressed.

Note that Sevaan asked for the strings to start with capitals in comment 19. Can you add a patch to change that please?

::: browser/components/loop/content/js/roomViews.jsx
@@ -286,2 @@
>              </p>
> -            <a className={cx({hide: !canAddContext, "room-invitation-addcontext": true})}

There's some room-invitation-addcontext classes that need removing:

http://mxr.mozilla.org/mozilla-central/search?string=room-invitation-addcontext

@@ +297,5 @@
> +              })}
> +              onClick={this.handleCopyButtonClick}>
> +              <img src="loop/shared/img/svg/glyph-link-16x16.svg" />
> +              <p>{mozL10n.get("invite_copy_" +
> +              (this.state.copiedUrl ? "triggered" : "button"))}</p>

nit: I think I'd prefer two-spaces more indent here.

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +273,2 @@
>  
> +      it("should keep the text soon after the url has been copied", function() {

nit: possibly s/soon/for a while/

Comment 31

[Mark Banner (:standard8)]

(Please make the l10n changes a separate patch, as we'll want the non-L10n stuff for uplift).

Comment 32

[Ed Lee :Mardak]

Attachment: for checkin

Comment 33

[Ed Lee :Mardak]

Attachment: strings update

Comment 34

[Ed Lee :Mardak]

Comment on attachment 8667685 [details] [diff] [review]
for checkin

I updated this patch with review comments and removed the .properties string deletion to a separate patch.

Comment 35

[Ed Lee :Mardak]

Comment on attachment 8667686 [details] [diff] [review]
strings update

Hrmm.. With the new uppercase string, it's the same as the one being removed, so the code should probably just use the old string:

invite_copy_triggered2=Copied!
copied_url_button=Copied!

And should we get rid of the other "triggered" strings as it seems like they won't be used? And "Share with contacts" too..

These are pretty close as well:

+invite_copy_button2=Copy link
 copy_url_button2=Copy Link

Comment 36

[Ed Lee :Mardak]

Attachment: using panel strings

Alternatively use the panel strings... which probably is not be good for l10n as the panel strings are used in a menu while the new functionality is labeled buttons.

Somewhat interesting to note: panel's menu strings are..

Copy Link
Email Link
Delete conversation

So there's some casing inconsistency here. And reusing the strings isn't quite right as I believe we want "Copy link" and "Email link"

Comment 37

[Mark Banner (:standard8)]

Firstly: Sevaan, should we use "Copy Link" (similar to what's in the menus in the panel for conversations), or "Copy link"?

I think this is a slightly different case as it isn't in a menu, so "Copy link" might make more sense, but I'll let Sevaan decide.

Secondly: Our strings here are a mess here. I would be happy for you to spin some or all of fixing this out to a separate bug (though we should make it high prio). I think what we should do is:

- Make the strings in the panel for the menus be consistent and next to each other in the file, e.g.

copy_link_menuitem
email_link_menuitem
delete_conversation_menuitem

- Make the conversation window strings consistent (append '2' if necessary), e.g.

invite_copy_link_button
invite_copied_link_button
...

I think button is about the right sort of term here, though we could use label/tooltip instead maybe (though its not quite any of those).

That will give L10n the context as to where these strings live, as there may be a few instances where they have to change the string.

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 8667686 [details] [diff] [review]
strings update

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

This is probably reasonably close to what we want, but I'll let you take a look at the comments & Sevaan's response when we get it, before I review this.

Comment 39

[Sevaan Franks [:sevaan]]

(In reply to Mark Banner (:standard8) from comment #37)
> Firstly: Sevaan, should we use "Copy Link" (similar to what's in the menus
> in the panel for conversations), or "Copy link"?

Copy Link, please.

Comment 40

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/63fe244f62b8ab4b7afe560b41a8bcf1a62afcaa
Bug 1184924 - Implement the refreshed design for the invitation overlay [r=Standard8]

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63fe244f62b8

Comment 42

[Ed Lee :Mardak]

Comment on attachment 8667686 [details] [diff] [review]
strings update

This patch was handled in bug 1210331.

Comment 43

[Ed Lee :Mardak]

Comment on attachment 8667730 [details] [diff] [review]
using panel strings

This patch was handled in bug 1210331.

Comment 44

[Ed Lee :Mardak]

Comment on attachment 8667685 [details] [diff] [review]
for checkin

This patch as attached is upliftable to aurora if bug 1209029 is *not* uplifted (there's merge conflicts with jar.mn and various glyph svgs being removed) and if bug 1211438 *is* uplifted (there's a potential merge conflict with conversation.css).

Standard8, do you want me to attach a patch that can be uplifted to mozilla-aurora right now? (I.e., without bug 1211438 uplifting first)

Comment 45

[Ed Lee :Mardak]

Attachment: for aurora (Mardak will land)

Approval Request Comment
[Feature/regressing bug #]: Hello Visual refresh
[User impact if declined]: Old text-button interface instead of new icons with matching colors.
[Describe test coverage new/current, TreeHerder]: Various unit tests landed on m-c and baked for about a week
[Risks and why]: Lowish - mostly css changes and reusing existing existing styles
[String/UUID change made/needed]: None - strings landed when m-c was 43

I'll land this after bug 1211438 is uplifted.

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8670936 [details] [diff] [review]
for aurora (Mardak will land)

Approved for aurora uplift, has tests, part of hello feature aimed at 43.

Comment 47

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is part of new feature work.

Comment 48

[Ed Lee :Mardak]

http://hg.mozilla.org/releases/mozilla-aurora/rev/6424691f9e9a
Bug 1184924 - Implement the refreshed design for the invitation overlay [r=Standard8, a=lizzard]

Comment 49

[Bogdan Maris, Desktop QA]

Verified that the new overlay works as expected using Firefox Developer Edition 43.0a2 and latest Nightly 44.0a1 across platforms (Windows 7 64-bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit).