Closed Bug 1208466 Opened 4 years ago Closed 4 years ago

When a link generator clicks their own link, make it open in the conversation window

Categories

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

defect
Points:
5

Tracking

(firefox44 verified)

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

People

(Reporter: standard8, Assigned: standard8)

References

Details

User Story

* If a desktop client user opens one of his own Hello URLs in Firefox and clicks the "JOIN" button when not being present in the conversation, the desktop client UI is used to join him to the conversation. The standalone link clicker UI then informs the user that he was joined successfully with the message "You can also open your conversation from the Hello panel."
Current mockups: (Before joining) http://i.sevaan.com/image/02263x1T0G2K; (After joining) http://i.sevaan.com/image/3I392T213l2J 
The words "Hello Panel" could be clicked on to open the panel itself.
* Send a Google Analytics event when a link clicker joins a conversation he created from the desktop client UI

Attachments

(7 files, 1 obsolete file)

Part of the work for bug 1154251, see the user story.
Rank: 22
This is a very rough unpolished first version that's fully functional wrt displaying a different view, providing a join button, and then an "enjoy your conversation" message.

Looking for some general feedback on the architecture whilst I start to flesh out the view styling and other things a bit more.
Attachment #8665982 - Flags: feedback?(mdeboer)
Depends on: 1208515
Sevaan, I need:

- SVG of mozilla logo
- SVG of Firefox Hello text (without the face bubble).
(In reply to Mark Banner (:standard8) from comment #2)
> Sevaan, I need:
> 
> - SVG of mozilla logo
> - SVG of Firefox Hello text (without the face bubble).

Now with the NI
Flags: needinfo?(sfranks)
Attached image Firefox Hello Wordmark
Flags: needinfo?(sfranks)
Attached image Mozilla Wordmark
Here's the "Join the conversation view". I've kept the "Mozilla" not at the bottom of the page, so that if the window is shorter than the space required, it can scroll nicely. I can add some more padding if you want.
Attachment #8666602 - Flags: ui-review?(sfranks)
This is what it looks like after clicking join (with conversation window open).
Attachment #8666603 - Flags: ui-review?(sfranks)
User Story: (updated)
This just separates out the ToS message from the footer into its own component. This way in the next patch we'll create a view that also uses the additional component.
Attachment #8665982 - Attachment is obsolete: true
Attachment #8665982 - Flags: feedback?(mdeboer)
Attachment #8666648 - Flags: review?(mdeboer)
This adds a new view for when the room will be handled via Firefox. It is now fully functional and tested.

I'm using a promise for the fetch - this ensures that both items have completed before we try moving on. This seemed a reasonable way of doing it (rather than trying to track individual states and then trigger the next room state once both had completed).
Attachment #8666650 - Flags: review?(mdeboer)
Comment on attachment 8666648 [details] [diff] [review]
Part 1. Create a new ToS view for Loop's standalone, ready for integration into the handled-in-Firefox views

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

LGTM!

::: browser/components/loop/test/standalone/standaloneRoomViews_test.js
@@ +98,5 @@
> +    it("should dispatch a link click action when the ToS link is clicked", function() {
> +      // [0] is the first link, the legal one.
> +      var link = node.querySelectorAll("a")[0];
> +
> +      TestUtils.Simulate.click(node, {target: link});

nit: plz add spaces inside the object literal.

@@ +111,5 @@
> +    it("should dispatch a link click action when the Privacy link is clicked", function() {
> +      // [0] is the first link, the legal one.
> +      var link = node.querySelectorAll("a")[1];
> +
> +      TestUtils.Simulate.click(node, {target: link});

nit: same.

@@ +121,5 @@
> +        }));
> +    });
> +
> +    it("should not dispatch an action when the text is clicked", function() {
> +      TestUtils.Simulate.click(node, {target: node});

nit: same.
Attachment #8666648 - Flags: review?(mdeboer) → review+
Comment on attachment 8666650 [details] [diff] [review]
Part 2. If an owner of a Loop link clicks their own link and join, make it open the conversation window.

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

Alright, I'm super pleased with this patch - great job!

r=me when you replace all notions of 'Firefox' with 'UserAgent' in this patch, because we try to not use brand names in our code. There are only a handful of nits. I'm really happy with the test coverage.

Thanks!

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +438,5 @@
> +     */
> +    _promiseDetectFirefoxHandles: function() {
> +      return new Promise(function(resolve, reject) {
> +        function resolveWithNotHandlingResponse() {
> +          console.log("called");

Hmm, you prolly want to remove this ;)

@@ +447,5 @@
>  
> +        // If we're not Firefox, don't even try to see if it can be handled
> +        // in the browser.
> +        if (!loop.shared.utils.isFirefox(navigator.userAgent)) {
> +          console.log("!isFirefox");

...and this.

@@ +643,5 @@
> +     */
> +    joinRoom: function() {
> +      // Reset the failure reason if necessary.
> +      if (this.getStoreState().failureReason) {
> +        this.setStoreState({failureReason: undefined});

nit: plz add spaces inside the object literal.

@@ +661,5 @@
> +      this.dispatcher.dispatch(new sharedActions.MetricsLogJoinRoom({
> +        firefoxHandledRoom: false
> +      }));
> +
> +      // otherwise, we handle the room ourselves.

Nit: plz start with an O
Attachment #8666650 - Flags: review?(mdeboer) → review+
Status: NEW → ASSIGNED
Comment on attachment 8666603 [details]
Enjoy your conversation view

I think the text in the disabled state should no longer be bold. Other than that +r
Attachment #8666603 - Flags: ui-review?(sfranks) → ui-review+
Attachment #8666602 - Flags: ui-review?(sfranks)
Fixes the ui-review, also fixes a missed bit during the search and replace - the standalone is currently always blank.
Attachment #8666719 - Flags: review?(mdeboer)
Comment on attachment 8666719 [details] [diff] [review]
Part 3. Fix display when opening room, and make the opened room text non-bold.

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

Whoops :)
Attachment #8666719 - Flags: review?(mdeboer) → review+
Flags: qe-verify+
QA Contact: bogdan.maris
Verified that the messages are correctly prompted before and after link generator joins it's conversation using latest Nightly 44.0a1 across platforms (Windows 8 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.