Closed Bug 1248568 Opened 8 years ago Closed 8 years ago

Terminating a web sharing session prompts the user to assign a name

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: RT, Assigned: fcampo)

References

()

Details

User Story

As a desktop client user, I want to have the option to name a room the first time I leave it so that my room list is more intuitive.

UX: http://i.sevaan.com/2W2J0I1m451N 

Acceptance criteria:
- As the user terminates the first instance of a share session he gets prompted to assign a name to it in a door hanger appearing off of the Hello button
- The default name offered is the HTML title of the last active page
- The user can edit the name field
- Pressing OK saves the share name
- Pressing anywhere else discards the action
- From now on, the name cannot be auto-updated although the Favicon can

Attachments

(3 files, 2 obsolete files)

      No description provided.
User Story: (updated)
Blocks: 1248602
Rank: 19
Priority: -- → P1
Summary: Terminating a web sharing session prompts the user to assign a name → [Meta] Terminating a web sharing session prompts the user to assign a name
Assignee: nobody → fernando.campo
Rank: 19 → 15
Sevaan, a few questions that came-up from discussions with engineering:
- How does this interact with the "Leave feedback" prompt?
- Do you feel we need a "Cancel" button if the user wants to keep having the room name change when the tabs shared change?
Flags: needinfo?(sfranks)
(In reply to Romain Testard [:RT] from comment #1)
> - How does this interact with the "Leave feedback" prompt?

We could leave the conversation window open with the Feedback stuff on it. Once you click okay on this panel, the conversation window would close as well.

> - Do you feel we need a "Cancel" button if the user wants to keep having the
> room name change when the tabs shared change?

No. If a user clicks Okay without making a change, it doesn't change anything and keeps the dynamic room name. I would suggest we look at how the string can better communicate this.
Flags: needinfo?(sfranks)
Fernando, we need this one completed by Thursday, can you please update on the progress or release the bug if this is not something you can work on currently?
Flags: needinfo?(fernando.campo)
I'm working on it but I don't think I'll finish before Thursday, got stuck on nightly testing problems.

WIP is on the branch https://github.com/fcampo/loop/tree/terminate-share-names-room-1248568, no patch ready yet.
Flags: needinfo?(fernando.campo)
Rank: 15 → 18
Attachment #8731852 - Flags: review?(standard8)
Attachment #8731852 - Flags: review?(dmose)
Attachment #8731852 - Flags: review?(crafuse)
Attached image UX (obsolete) —
Attachment #8731853 - Flags: ui-review?(sfranks)
Attachment #8731853 - Flags: ui-review?(b.pmm)
Attached image UX:focus (obsolete) —
Attachment #8731855 - Flags: review?(sfranks)
Attachment #8731855 - Flags: review?(b.pmm)
Attachment #8731852 - Flags: review?(dmose)
Attachment #8731852 - Flags: review?(crafuse)
This is an additional patch to go with the PR. As mentioned there I found a few issues in testing the patch. I addressed them in a patch as it was easier than trying to explain in the PR. However, here's a summary of the changes:

- onRoomAdded gets called on startup when the initial getAll is triggered, therefore it seems a dangerous to be setting the lastCreateRoom there. So I moved this to the createRoom function.

This should also mean that if there's multiple windows open we'll only get the rename room message in the panel that created the room initially.

- In onRoomClosed, state.lastCreatedRoom and state.openedRoom can both be null, so it seems worth a check that we don't show the panel in this case.

- As your new close panel is displayed without the room list behind it, it unmounts the room list. When the room list is next displayed, it triggers another getAll for the rooms, for which the store adds another bunch of listeners.

So when we next close a room, the panel was opening again due to the null == null of the previous case.

We shouldn't be adding multiple listeners there. We still need to tidy up how it works in a later bug, but preventing the getAll happening again is fine for now.

Fernando, if you're happy with the patch, please can you add it to the PR?
Flags: needinfo?(fernando.campo)
Summary: [Meta] Terminating a web sharing session prompts the user to assign a name → Terminating a web sharing session prompts the user to assign a name
Attachment #8731852 - Flags: review?(standard8)
Comment on attachment 8731853 [details]
UX

Hmm, something doesn't look right to me here. Is this using the system font?

Can you confirm that the title is 16 an the body text is 13?

Also, the line spacing between the title and the body should match the spacing between the body text and "Current name:"

Supplied design for comparison: http://i.sevaan.com/3C08433f3f12
Attachment #8731853 - Flags: ui-review?(sfranks)
Attachment #8731853 - Flags: ui-review?(b.pmm)
Attachment #8731853 - Flags: ui-review-
Attachment #8731855 - Flags: review?(sfranks)
Attachment #8731855 - Flags: review?(b.pmm)
Attachment #8731855 - Flags: review-
(In reply to Sevaan Franks [:sevaan] from comment #9)
> Comment on attachment 8731853 [details]
> UX
> 
> Hmm, something doesn't look right to me here. Is this using the system font?
I didn't change any font, so it's using the default one. 
If it needs to be something specific, please add the details to the spec image.

> 
> Can you confirm that the title is 16 an the body text is 13?
px or pt? right now is 1.2em, that should be equivalent to 16px
16pt would be around 21.5px
same comparison with body, which is 13px atm.
> 
> Also, the line spacing between the title and the body should match the
> spacing between the body text and "Current name:"
Sure, the h1 has some extra spacing, but I can reduce it.
Can I have some specific values? there wasn't any on the image
> 
> Supplied design for comparison: http://i.sevaan.com/3C08433f3f12
Adding the values for empty spaces (margins/paddings), sizes for elements/fonts (specially the icon) and specific details would be very useful :)
Flags: needinfo?(fernando.campo) → needinfo?(sfranks)
This should clarify some things: http://i.sevaan.com/3v3g2S46030u

re: fonts I was talking px, which you seem to have implemented. Maybe it's just the margins throwing things off.
Flags: needinfo?(sfranks)
Blocks: 1258334
No longer blocks: 1248602
Rank: 18 → 9
Attached image rename-panel.png
updated margins/sizes
Attachment #8731853 - Attachment is obsolete: true
Attachment #8731855 - Attachment is obsolete: true
Attachment #8733993 - Flags: ui-review?(sfranks)
Comment on attachment 8733993 [details]
rename-panel.png

Looks great, thanks!
Attachment #8733993 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8731852 [details] [review]
[loop] fcampo:terminate-share-names-room-1248568 > mozilla:master

- your patch added
- UI fixed
- comments addressed
- extra tests \o/
Attachment #8731852 - Flags: review?(standard8)
Comment on attachment 8731852 [details] [review]
[loop] fcampo:terminate-share-names-room-1248568 > mozilla:master

Looks good, r=Standard8 with the final few comments addressed.
Attachment #8731852 - Flags: review?(standard8) → review+
merged https://github.com/mozilla/loop/commit/116d121762cc03b5d3e3557d11a812fc7b939c95
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 1265865
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: