1245147 - Inform the link clicker when tab sharing is paused

Status: RESOLVED FIXED

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

Comment 1

[Romain Testard [:RT]]

Sevaan can you please provide the UX?

Comment 2

[Pau Masiá [:Pau]]

Attachment: FirefoxHello_LinkClicker_Paused.png

Here's the UX for displaying this message when the room creator pauses sharing their tabs.

Assets and specs to follow.

Comment 3

[Pau Masiá [:Pau]]

I'll upload an updated version to match what's possible to do in 1.2

Comment 4

[Sevaan Franks [:sevaan]]

Attachment: Link Clicker - Paused

Comment 5

[Pau Masiá [:Pau]]

Attachment: Assets.zip

Pause icon asset.

Comment 6

[Romain Testard [:RT]]

Thanks, we cannot get the background image so just to be clear we'll have to implement for now (until we have the snapshot feature done) a black screen with the message and visual asset provided in the mocks.

Comment 7

[Sevaan Franks [:sevaan]]

Does it have to be black? Or can the stream display a different solid colour or even an image?

Comment 8

[Romain Testard [:RT]]

At that point we would not display the stream. The clicker UI receives an event and displays what we need, the limitation is just that we cannot display the website that was shared, I guess it could be any color or image though.

Comment 9

[Pau Masiá [:Pau]]

Let's stick with this background color #D4D4D4 for now, which is the one we have in every error screen, empty state, and work to have a better, more fun, engaging background option for the future.

Comment 10

[Romain Testard [:RT]]

OK, US field updated accordingly

Comment 11

[GitHub Autolander Bot]

Attachment: [loop] mancas:bug1245147 > mozilla:master

Comment 12

[Manuel Casas Barrado [:mancas]]

Attachment: Screen sharing paused notification

Comment 13

[Sevaan Franks [:sevaan]]

Comment on attachment 8720186 [details]
Screen sharing paused notification

Slightly too much padding. The top and bottom should be 10px, and the left and right should be 15px.

Comment 14

[Manuel Casas Barrado [:mancas]]

Attachment: Screen sharing paused notification

Comment 15

[Sevaan Franks [:sevaan]]

Comment on attachment 8720295 [details]
Screen sharing paused notification

Almost there. Just the right-side padding needs correcting. +r with that change.

Comment 16

[Manuel Casas Barrado [:mancas]]

Attachment: Screen sharing paused notification

Fixed! Thank you Sevaan

Comment 17

[Sevaan Franks [:sevaan]]

Comment on attachment 8720314 [details]
Screen sharing paused notification

Thank you!

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8719459 [details] [review]
[loop] mancas:bug1245147 > mozilla:master

I'm not sure whether adding the state in the cursorStore is the right place...

I'm also not really fond of the fact that we (re-)added a dependency on BackBone.Events... AFAIK we're trying to rip it out.
Why wasn't a storeMixin registered for the remoteCursorStore? BaseStore already has BackBone.Events mixed in; why aren't we using that?

Comment 19

[Mike de Boer [:mikedeboer]]

I forwarded this review request to Mark, because I don't feel I'm the best reviewer for this patch atm.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8719459 [details] [review]
[loop] mancas:bug1245147 > mozilla:master

I've just put a comment on the PR about how we're architecting it. Its open for discussion so not set in stone, but I like to know its been considered.

Comment 21

[Mark Banner (:standard8)]

Also, the PR didn't work straight out the box, as the line to listen for the action had been added in the wrong place. It took me a while to find that.

Comment 22

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8719459 [details] [review]
[loop] mancas:bug1245147 > mozilla:master

Hey Mark, I've implemented a new approach based on your comments. Can you review tha patch?

Thank you.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8719459 [details] [review]
[loop] mancas:bug1245147 > mozilla:master

That's better, I like the new architecture.

I've left a few things that still need to be addressed on the PR.

Comment 24

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8719459 [details] [review]
[loop] mancas:bug1245147 > mozilla:master

Comments addressed

Comment 25

[Manuel Casas Barrado [:mancas]]

Attachment: Screen sharing paused notification RTL

Comment 26

[Sevaan Franks [:sevaan]]

Comment on attachment 8724799 [details]
Screen sharing paused notification RTL

I spoke to the folks in #RTL on IRC, to make sure this is correct, and they said it looks good. However, it would be preferable to mirror the icon as well, so the speech bubble point is coming from the other side.

So, let's do that.

Also, seeing the full screenshot here...do you know if the logo Localizes as well, and puts the Hello face on the other side (maybe mirrored as well)?

Comment 27

[Mark Banner (:standard8)]

(In reply to Sevaan Franks [:sevaan] from comment #26)
> Also, seeing the full screenshot here...do you know if the logo Localizes as
> well, and puts the Hello face on the other side (maybe mirrored as well)?

It doesn't. We only have the one image for that available which is the combined logo and image.

Comment 28

[Manuel Casas Barrado [:mancas]]

Attachment: Screen sharing paused notification RTL

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 8719459 [details] [review]
[loop] mancas:bug1245147 > mozilla:master

This is much better, thanks for the additions. r=Standard8

Comment 30

[Manuel Casas Barrado [:mancas]]

Landed in master: https://github.com/mozilla/loop/commit/5f499ea46cc4d2df23e10e8ef4a3a5918224c509