Inform the link clicker when tab sharing is paused

RESOLVED FIXED

Status

Hello (Loop)
Client
P2
normal
Rank:
26
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: RT, Assigned: mancas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [btpp-active])

User Story

Acceptance criteria:
- Rather than show a black screen when tab sharing is paused, a message should inform the link clicker about what is going on.
Message to display: "Your friend has stopped sharing their tabs" over a background with color #D4D4D4

Attachments

(5 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

3 years ago
User Story: (updated)
(Reporter)

Comment 1

3 years ago
Sevaan can you please provide the UX?
Flags: needinfo?(sfranks)
Rank: 26
Priority: -- → P2

Comment 2

3 years ago
Created attachment 8715327 [details]
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.
Flags: needinfo?(sfranks)

Comment 3

3 years ago
I'll upload an updated version to match what's possible to do in 1.2
Created attachment 8715843 [details]
Link Clicker - Paused
Attachment #8715327 - Attachment is obsolete: true

Comment 5

3 years ago
Created attachment 8715861 [details]
Assets.zip

Pause icon asset.
(Reporter)

Comment 6

3 years ago
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.
Does it have to be black? Or can the stream display a different solid colour or even an image?
Flags: needinfo?(rtestard)
(Reporter)

Comment 8

3 years ago
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.
Flags: needinfo?(rtestard)

Comment 9

3 years ago
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.
(Reporter)

Comment 10

3 years ago
OK, US field updated accordingly
User Story: (updated)
(Assignee)

Updated

2 years ago
Assignee: nobody → b.mcb
Created attachment 8719459 [details] [review]
[loop] mancas:bug1245147 > mozilla:master
(Assignee)

Updated

2 years ago
Attachment #8719459 - Flags: review?(mdeboer)
(Assignee)

Comment 12

2 years ago
Created attachment 8720186 [details]
Screen sharing paused notification
Attachment #8720186 - Flags: ui-review?(sfranks)
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.
Attachment #8720186 - Flags: ui-review?(sfranks) → ui-review-
(Assignee)

Comment 14

2 years ago
Created attachment 8720295 [details]
Screen sharing paused notification
Attachment #8720186 - Attachment is obsolete: true
Attachment #8720295 - Flags: ui-review?(sfranks)
Comment on attachment 8720295 [details]
Screen sharing paused notification

Almost there. Just the right-side padding needs correcting. +r with that change.
Attachment #8720295 - Flags: ui-review?(sfranks) → ui-review-
(Assignee)

Comment 16

2 years ago
Created attachment 8720314 [details]
Screen sharing paused notification

Fixed! Thank you Sevaan
Attachment #8720314 - Flags: ui-review?(sfranks)
(Assignee)

Updated

2 years ago
Attachment #8720295 - Attachment is obsolete: true
Comment on attachment 8720314 [details]
Screen sharing paused notification

Thank you!
Attachment #8720314 - Flags: ui-review?(sfranks) → ui-review+
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?
Attachment #8719459 - Flags: review?(standard8)
Attachment #8719459 - Flags: review?(mdeboer)
Attachment #8719459 - Flags: feedback+
I forwarded this review request to Mark, because I don't feel I'm the best reviewer for this patch atm.
Whiteboard: [btpp-active]
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.
Attachment #8719459 - Flags: review?(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.
(Assignee)

Comment 22

2 years ago
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.
Attachment #8719459 - Flags: review?(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.
Attachment #8719459 - Flags: review?(standard8)
(Assignee)

Comment 24

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

Comments addressed
Attachment #8719459 - Flags: review?(standard8)
(Assignee)

Comment 25

2 years ago
Created attachment 8724799 [details]
Screen sharing paused notification RTL
Attachment #8724799 - Flags: ui-review?(sfranks)
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)?
Attachment #8724799 - Flags: ui-review?(sfranks) → ui-review-
(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.
(Assignee)

Comment 28

2 years ago
Created attachment 8725138 [details]
Screen sharing paused notification RTL
Attachment #8724799 - Attachment is obsolete: true
Attachment #8725138 - Flags: ui-review?(sfranks)
Attachment #8725138 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8719459 [details] [review]
[loop] mancas:bug1245147 > mozilla:master

This is much better, thanks for the additions. r=Standard8
Attachment #8719459 - Flags: review?(standard8) → review+
(Assignee)

Comment 30

2 years ago
Landed in master: https://github.com/mozilla/loop/commit/5f499ea46cc4d2df23e10e8ef4a3a5918224c509
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.