Closed Bug 1184924 Opened 5 years ago Closed 4 years ago

Implement the refreshed design for the invitation overlay

Categories

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

defect
Points:
5

Tracking

(firefox43+ verified, firefox44+ verified)

VERIFIED FIXED
mozilla44
Iteration:
44.1 - Oct 5
Tracking Status
firefox43 + verified
firefox44 + verified

People

(Reporter: mikedeboer, Assigned: Mardak)

References

Details

(Whiteboard: [visual refresh][strings])

Attachments

(7 files, 9 obsolete files)

2.50 KB, patch
dmose
: review+
Details | Diff | Splinter Review
145.93 KB, image/png
Details
53.72 KB, video/webm
sevaan
: ui-review+
Details
6.49 KB, application/zip
Details
155.23 KB, image/png
Details
34.59 KB, patch
Details | Diff | Splinter Review
34.33 KB, patch
Details | Diff | Splinter Review
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.
Flags: qe-verify+
Flags: firefox-backlog+
Rank: 19
Rank: 19 → 17
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 43.1 - Aug 24
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.)
Flags: needinfo?(sfranks)
(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
Flags: needinfo?(sfranks)
(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...
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.
Just for clarification, the "Submit Feedback" option in the gear menu on http://i.sevaan.com/image/393d2T103U2F is not required.
Iteration: 43.1 - Aug 24 → 43.2 - Sep 7
Blocks: 1199138
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 43.2 - Sep 7 → ---
Blocks: 1178304
Rank: 17 → 14
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.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Whiteboard: [visual refresh] → [visual refresh][strings]
ni? for the images for the buttons:

https://www.dropbox.com/sh/46pyq5wwgnif6g8/AACEqjLwDw1be0oMYw-okHA9a?dl=0&preview=Share_Screen.png
Flags: needinfo?(sfranks)
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.
Flags: needinfo?(sfranks) → needinfo?(mmaslaney)
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?
Flags: needinfo?(sfranks)
No, no need to implement that. It appears Contacts will be going away in forthcoming releases.
Flags: needinfo?(sfranks)
Attached image wip screenshot (obsolete) —
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.)
Flags: needinfo?(sfranks)
Attached patch wip (obsolete) — Splinter Review
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).
Attachment #8663085 - Flags: review?(dmose)
Comment on attachment 8663085 [details] [diff] [review]
added strings patch

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

rs=dmose
Attachment #8663085 - Flags: review?(dmose) → review+
https://hg.mozilla.org/integration/fx-team/rev/9d8166cf45a2bc6b1c5d1dab4dc6dc37214dad0d
Bug 1184924 - Implement the refreshed design for the invitation overlay strings [rs=dmose]
Keywords: leave-open
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.
(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.
Flags: needinfo?(sfranks)
Attached image old invite spec (obsolete) —
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.
(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.
Attached video 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.
Attachment #8663052 - Attachment is obsolete: true
Attachment #8664114 - Flags: ui-review?(sfranks)
Attached patch wip v2 (obsolete) — Splinter Review
Attachment #8663053 - Attachment is obsolete: true
Attachment #8663694 - Attachment is obsolete: true
Attached file Invitation Assets (SVG) (obsolete) —
Assets from :mmaslaney attached.
Flags: needinfo?(mmaslaney)
Attachment #8664114 - Flags: ui-review?(sfranks) → ui-review+
(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.
Flags: needinfo?(mmaslaney)
New assets attached
Flags: needinfo?(mmaslaney)
Attached image v1 screenshot
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #27)
> Created attachment 8664377 [details]
> Hello_Assets_092015-v2.zip
> New assets attached
Thanks! Looks good!
Attachment #8664198 - Attachment is obsolete: true
Attachment #8664301 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
Attachment #8664116 - Attachment is obsolete: true
Attachment #8664748 - Flags: review?(standard8)
Keywords: leave-open
Iteration: --- → 44.1 - Oct 5
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/
Attachment #8664748 - Flags: review?(standard8) → review+
(Please make the l10n changes a separate patch, as we'll want the non-L10n stuff for uplift).
Attached patch for checkinSplinter Review
Attached patch strings update (obsolete) — Splinter Review
Attachment #8664748 - Attachment is obsolete: true
Attachment #8667686 - Flags: review?(standard8)
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 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
Attached patch using panel strings (obsolete) — Splinter Review
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"
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.
Flags: needinfo?(sfranks)
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.
Attachment #8667686 - Flags: review?(standard8)
(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.
Flags: needinfo?(sfranks)
https://hg.mozilla.org/integration/fx-team/rev/63fe244f62b8ab4b7afe560b41a8bcf1a62afcaa
Bug 1184924 - Implement the refreshed design for the invitation overlay [r=Standard8]
Blocks: 1210331
https://hg.mozilla.org/mozilla-central/rev/63fe244f62b8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1211592
Blocks: 1211563
Comment on attachment 8667686 [details] [diff] [review]
strings update

This patch was handled in bug 1210331.
Attachment #8667686 - Attachment is obsolete: true
Comment on attachment 8667730 [details] [diff] [review]
using panel strings

This patch was handled in bug 1210331.
Attachment #8667730 - Attachment is obsolete: true
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)
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.
Attachment #8670936 - Flags: approval-mozilla-aurora?
Depends on: 1211438
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.
Attachment #8670936 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking since this is part of new feature work.
http://hg.mozilla.org/releases/mozilla-aurora/rev/6424691f9e9a
Bug 1184924 - Implement the refreshed design for the invitation overlay [r=Standard8, a=lizzard]
QA Contact: bogdan.maris
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.