Closed Bug 1221266 Opened 4 years ago Closed 4 years ago

Get Loop into shape for releasing in 44 - correct terminology & re-allow the let's talk about context for rooms

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(firefox44+ verified, firefox45 wontfix)

VERIFIED FIXED
Tracking Status
firefox44 + verified
firefox45 --- wontfix

People

(Reporter: standard8, Assigned: Mardak)

Details

User Story

- Revert "Browse this page with a friend" to "Start a conversation"
- Change "Stop sharing your tabs" to "End conversation"
- Revert "RECENTLY BROWSED" to "CURRENT CONVERSATIONS"
- Change "CURRENTLY BROWSING" to "CURRENT CONVERSATION"
- On the panel, reintroduce the "Let's talk about" section for attaching context to a conversation. This can be above the start a conversation button and the button can stay at the top of the panel (reverting bug 1212331 might do most of the work here).

Attachments

(8 files, 1 obsolete file)

Due to the early merge and some discussions, Hello in FF 44 isn't suitable for release yet.

See the user story for what needs to change.

Sevaan: Please can you fill in the XXXs and correct anything I've missed.
Flags: needinfo?(sfranks)
> - Change "Stop sharing your tabs" to "XXX"

End Conversation

> - Change "CURRENTLY BROWSING" to "XXX"

Current Conversation
Flags: needinfo?(sfranks)
User Story: (updated)
 <sevaan> All caps. Same format as the previous strings...just the new words.
User Story: (updated)
Attached video v1 video
Attachment #8682741 - Flags: ui-review?(sfranks)
Attached patch v1Splinter Review
Added new strings and backed out the context removal patch (fixing up conflicts: button/context location+styling, test createTestComponent for inRoom flag, ui showcase)
Attachment #8682759 - Flags: review?(standard8)
One piece of "new" code: don't show the context if the user is in a room.
I think that's probably okay to keep.

Do we have stats on how many people who are currently in a conversation try to start a new one?
Comment on attachment 8682741 [details]
v1 video

Is it possible to have the Add Context above the button?
Attachment #8682741 - Flags: ui-review?(sfranks) → ui-review-
Attached patch v1.1 delta context on top (obsolete) — Splinter Review
Attached image v1.1 screenshot
Is the active background okay? I made it 1rem/10px above/below the context and 1.5rem/15px on the sides. And the same spacing around the button.
Attachment #8682907 - Flags: ui-review?(sfranks)
[Tracking Requested - why for this release]: Due to the early merge and some late decision making, Hello isn't in a suitable position to ship - some of the text implies things will happen that aren't going to happen (e.g. tab sharing), and the "let's talk about" option had been removed expecting that we'd have more feature rework complete.

We've decided that rather than backing out everything from the 44 cycle, it would be better to apply the minor fixes proposed in this bug. This will have string impact, hence flagging early for tracking purposes.
Comment on attachment 8682759 [details] [diff] [review]
v1

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

Looks good so far. I'll r+ this one, and the second part I can look at once the UX is agreed.
Attachment #8682759 - Flags: review?(standard8) → review+
Comment on attachment 8682907 [details]
v1.1 screenshot

I think originally the active background surrounded the button as well, to communicate that the context is being attached to the button when you turn it on.
Attached image beta43 screenshot
Attached image v1.2 screenshot
This is slightly different from what existed in 43 (attachment 8683131 [details]), but it looks fine. Just need to double check the expected spacing then.
Attachment #8683132 - Flags: ui-review?(sfranks)
Attached image v1.2b screenshot
Tweaked the spacing between context/button to be the same as the spacing above context and below button.
Attachment #8683135 - Flags: ui-review?(sfranks)
Attachment #8682906 - Attachment is obsolete: true
Attachment #8683136 - Flags: review?(standard8)
Comment on attachment 8683135 [details]
v1.2b screenshot

Right, it didn't surround the button before, but it was still aligned to the button edges, associating the checkedbox to the content.

I think this implementation is okay though if we are keeping the button at the top.
Attachment #8683135 - Flags: ui-review?(sfranks) → ui-review+
Comment on attachment 8683132 [details]
v1.2 screenshot

I prefer your fixed spacing. Thanks for using your design eye!
Attachment #8683132 - Flags: ui-review?(sfranks) → ui-review-
Attachment #8682907 - Flags: ui-review?(sfranks) → ui-review-
Approval Request Comment
[Feature/regressing bug #]: Hello web sharing bug 1209713
[User impact if declined]: A partially implemented new experience with strings that assume a completed experience would be confusing. Inability to control context of a conversation (where the new experience would have automatically updated the context).
[Describe test coverage new/current, TreeHerder]: Tests restored/unbackedout from bug 1212331. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d513ba956066
[Risks and why]: Code is mostly restoring existing functionality that has been previously tested/actively used. Other changes are minor css and string changes.
[String/UUID change made/needed]: Strings. 4 new strings that match the partially implemented experience.
Attachment #8683141 - Flags: approval-mozilla-aurora?
Attachment #8683136 - Flags: review?(standard8) → review+
Ed Lee, does this patch need to land on Nightly45 first? Just wondering.
Flags: needinfo?(edilee)
No, this is a special patch just for 44.
Flags: needinfo?(edilee)
I hope you understand how confusing is to have strings that land in mozilla-aurora without being in mozilla-central, and over a week after merge day.

It will mess with localizers working on l10n-central (the ones I care more about, given that we have fewer and fewer of them). In the meantime, people working on aurora already removed those strings.
(In reply to Francesco Lodolo [:flod] from comment #23)
> I hope you understand how confusing is to have strings that land in
> mozilla-aurora without being in mozilla-central, and over a week after merge
> day.

Please see comment 0 for the reasons for doing this - unfortunately circumstances around the merges caught us out.

Note also, that the approval request was made the day after merge day and I'd also notified drivers over irc last Friday.
Comment on attachment 8683141 [details] [diff] [review]
for aurora uplift

Let's uplift to Aurora44.
Attachment #8683141 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b5bdeedf0f4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Flags: qe-verify+
We did some exploratory testing around the fixes here following the user story using Firefox 44 beta 2 across platforms (Windows 7 64-bit, Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 64-bit) and can confirm that the feature looks good for Firefox 44.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.