Closed Bug 1074674 Opened 5 years ago Closed 5 years ago

As a user, I should be able to easily copy a room location to the clipboard, so that I don't have to manually copy

Categories

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

enhancement

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.2
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: dmose)

References

Details

(Whiteboard: [rooms][strings])

User Story

* As a user, I should be able to easily copy a room location to the clipboard, so that I don't have to manually copy

UX:
* Add a copy button to the room list view that shows on hover, as per http://people.mozilla.org/~dhenein/loop/rooms/room-hover.html
* When the copy button is clicked:
** the url is copied to the clipboard
** the button changes to a "tick"

Strings:
* Tooltip: Copy Link

Implemention:
* Implement a copy button that copies to the clipboard - use the mozLoop.copyString API

Tech checklist
==

* test when practical & implement 

** hovering list entry displays a copy icon
** blurring list entry removes copy icon
** clicking does the copy
*** (presumably by dispatching an action)
** icon resets to unchecked on blur
** clicking switches to check icon
** displaying check-icon animates a bulge
** hovering animates in the copy icon

Attachments

(3 files, 2 obsolete files)

Attached image Expected UX - Hover
No description provided.
User Story: (updated)
Attached image Expected UX - Clicked
Severity: normal → enhancement
User Story: (updated)
backlog: --- → Fx35+
Assignee: nobody → dmose
Depends on: 1079283
Updated hover animation link to one that works.
User Story: (updated)
Attachment #8512132 - Flags: review?(nperriault)
Priority: -- → P1
Comment on attachment 8512132 [details] [diff] [review]
add button to copy room location to clipboard

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

This looks super good! Some issues we may want to address before landing though.

::: browser/components/loop/content/js/panel.jsx
@@ +478,3 @@
>      shouldComponentUpdate: function(nextProps, nextState) {
> +      return (nextProps.room.ctime > this.props.room.ctime) ||
> +        (nextState.urlCopied != this.state.urlCopied);

Nit: strict equality check please (for consistency rather than actual technical reasons).

::: browser/components/loop/content/shared/img/checkmark-16x16.svg
@@ +6,5 @@
> +  <circle fill-rule="evenodd" clip-rule="evenodd" fill="#0096DD" cx="8"
> +          cy="8" r="8"/>
> +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#FFFFFF"
> +        d="M7.236,12L12,5.007L10.956,4L7.224,9.465l-2.14-2.326L4,8.146L7.236,12z"/>
> +</svg>

Any reason this hasn't been added to the existing icons-16x16.svg file?

::: browser/components/loop/content/shared/img/copy-16x16.svg
@@ +24,5 @@
> +      <polygon fill-rule="evenodd" clip-rule="evenodd" fill="#FFFFFF"
> +               points="6.963,5.714 6.963,4.571 8.148,5.714"/>
> +    </g>
> +  </g>
> +</svg>

Any reason this hasn't been added to the existing icons-16x16.svg file?

::: browser/components/loop/jar.mn
@@ +34,5 @@
>    content/browser/loop/shared/img/loading-icon.gif              (content/shared/img/loading-icon.gif)
>    content/browser/loop/shared/img/audio-inverse-14x14.png       (content/shared/img/audio-inverse-14x14.png)
>    content/browser/loop/shared/img/audio-inverse-14x14@2x.png    (content/shared/img/audio-inverse-14x14@2x.png)
> +  content/browser/loop/shared/img/copy-16x16.svg		(content/shared/img/copy-16x16.svg)
> +  content/browser/loop/shared/img/checkmark-16x16.svg             (content/shared/img/checkmark-16x16.svg)

There's a svg/ subdirectory in shared/img, it may be worth moving svg files in there.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +641,5 @@
> +        roomToken: "QzBbvGmIZWU",
> +        roomUrl: "http://sample/QzBbvGmIZWU",
> +        roomName: "Second Room Name",
> +        maxSize: 2,
> +        currSize: 0,

Note that currSize has been deprecated and is replaced by a `participants` array[0]; though I suppose this could land and we'll fix the issue in a separate bug (bug 1074672 has the fix and should land soon).

[0]: https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms.2F.7Btoken.7D

@@ +654,5 @@
> +        room: roomStore
> +      }));
> +    }
> +
> +    it("should display copy-link button by default",

should *not*.

@@ +655,5 @@
> +      }));
> +    }
> +
> +    it("should display copy-link button by default",
> +      function() {

Nit: unneeded indentation.

@@ +680,5 @@
> +      var buttonNode = roomEntry.getDOMNode().querySelector("button.copy-link");
> +
> +      TestUtils.Simulate.click(buttonNode);
> +
> +      roomEntry.forceUpdate();

This is unnecessary and can be safely removed.

@@ +688,5 @@
> +    it("should switch to displaying a check icon when the URL has been copied",
> +      function(done) {
> +        var roomEntry = mountRoomEntry();
> +
> +        roomEntry.setState({urlCopied: true}, function() {

Why setting state manually rather than just clicking the button, as you did in the previous test?

@@ +695,5 @@
> +          expect(buttonNode).to.not.equal(null);
> +
> +          done();
> +        });
> +    });

This test can be written a simpler way, like:

it("should switch to displaying a check icon when the URL has been copied",
  function() {
    var roomEntry = mountRoomEntry();
    var buttonNode = roomEntry.getDOMNode().querySelector("button.copy-link");

    TestUtils.Simulate.click(buttonNode);

    expect(buttonNode.classList.contains("checked")).eql(true);
});

@@ +698,5 @@
> +        });
> +    });
> +
> +    // XXX remove the .skip here (and do any necessary debugging), once
> +    // https://github.com/facebook/react/issues/1297 is fixed.

Have you investigated the workaround mentioned in this comment? https://github.com/facebook/react/issues/1297#issuecomment-46623923

Here's a working example:

it("should not display a check icon after mouse leaves the entry", function() {
  var roomEntry = mountRoomEntry();
  var roomNode = roomEntry.getDOMNode();
  var buttonNode = roomEntry.getDOMNode().querySelector("button.copy-link");
  TestUtils.Simulate.click(buttonNode);

  TestUtils.SimulateNative.mouseOut(roomNode);

  expect(buttonNode.classList.contains("checked")).eql(false);
});
Attachment #8512132 - Flags: review?(nperriault) → feedback+
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #6)

> This looks super good! Some issues we may want to address before landing
> though.

:-)  Thanks!

In general, it's unusual to request review? and rather than finding a r+ or r-, finding an f+ instead, meaning that it's hard to interpret.  so it's probably worth being explicit when you do this.  Is the intent here that you want to see it again before landing (typically, this is expressed with r-)? Or that it can land once I've addressed stuff as I see fit (typically, "r+ on the condition that...")?  Or that you want someone else to review this too?

> ::: browser/components/loop/content/js/panel.jsx
> @@ +478,3 @@
> >      shouldComponentUpdate: function(nextProps, nextState) {
> > +      return (nextProps.room.ctime > this.props.room.ctime) ||
> > +        (nextState.urlCopied != this.state.urlCopied);
> 
> Nit: strict equality check please (for consistency rather than actual
> technical reasons).

Fixed.

> ::: browser/components/loop/content/shared/img/checkmark-16x16.svg
> @@ +6,5 @@
> > +  <circle fill-rule="evenodd" clip-rule="evenodd" fill="#0096DD" cx="8"
> > +          cy="8" r="8"/>
> > +  <path fill-rule="evenodd" clip-rule="evenodd" fill="#FFFFFF"
> > +        d="M7.236,12L12,5.007L10.956,4L7.224,9.465l-2.14-2.326L4,8.146L7.236,12z"/>
> > +</svg>
> 
> Any reason this hasn't been added to the existing icons-16x16.svg file?
> 
> ::: browser/components/loop/content/shared/img/copy-16x16.svg
> @@ +24,5 @@
> > +      <polygon fill-rule="evenodd" clip-rule="evenodd" fill="#FFFFFF"
> > +               points="6.963,5.714 6.963,4.571 8.148,5.714"/>
> > +    </g>
> > +  </g>
> > +</svg>
> 
> Any reason this hasn't been added to the existing icons-16x16.svg file?

Some combination of me forgetting about that file, and the fact that we are already using a bunch of other individual svg files.  I checked with a few folks in fx-team, and the consensus seemed to be that it can be nice for organizational reasons, particularly if a bunch of CSS is shared between the icons, but it's not a general always-do-it-this-way sort of thing.

In this case, it seems like extra work to go reformat and test them to go in that one file, and it doesn't seem like there's a big organizational benefit here, so...

> ::: browser/components/loop/jar.mn
> @@ +34,5 @@
> >    content/browser/loop/shared/img/loading-icon.gif              (content/shared/img/loading-icon.gif)
> >    content/browser/loop/shared/img/audio-inverse-14x14.png       (content/shared/img/audio-inverse-14x14.png)
> >    content/browser/loop/shared/img/audio-inverse-14x14@2x.png    (content/shared/img/audio-inverse-14x14@2x.png)
> > +  content/browser/loop/shared/img/copy-16x16.svg		(content/shared/img/copy-16x16.svg)
> > +  content/browser/loop/shared/img/checkmark-16x16.svg             (content/shared/img/checkmark-16x16.svg)
> 
> There's a svg/ subdirectory in shared/img, it may be worth moving svg files
> in there.

...for this iteration of the patch, I've just moved the SVG files here.

> ::: browser/components/loop/test/desktop-local/panel_test.js
> @@ +641,5 @@
> > +        roomToken: "QzBbvGmIZWU",
> > +        roomUrl: "http://sample/QzBbvGmIZWU",
> > +        roomName: "Second Room Name",
> > +        maxSize: 2,
> > +        currSize: 0,
> 
> Note that currSize has been deprecated and is replaced by a `participants`
> array[0];

Fixed.

> @@ +654,5 @@
> > +        room: roomStore
> > +      }));
> > +    }
> > +
> > +    it("should display copy-link button by default",
> 
> should *not*.

Fixed.

> @@ +655,5 @@
> > +      }));
> > +    }
> > +
> > +    it("should display copy-link button by default",
> > +      function() {
> 
> Nit: unneeded indentation.

Fixed.

> @@ +680,5 @@
> > +      var buttonNode = roomEntry.getDOMNode().querySelector("button.copy-link");
> > +
> > +      TestUtils.Simulate.click(buttonNode);
> > +
> > +      roomEntry.forceUpdate();
> 
> This is unnecessary and can be safely removed.

Are you sure?  All that the click handler does is call this.setState(), and http://facebook.github.io/react/docs/component-api.html sez "There is no guarantee of synchronous operation of calls to setState".

> @@ +688,5 @@
> > +    it("should switch to displaying a check icon when the URL has been copied",
> > +      function(done) {
> > +        var roomEntry = mountRoomEntry();
> > +
> > +        roomEntry.setState({urlCopied: true}, function() {
> 
> Why setting state manually rather than just clicking the button, as you did
> in the previous test?

For the same reason I used forceUpdate in the previous test: so that I can know that the setState has completed, given that synchronous operation is not guaranteed.

> @@ +695,5 @@
> > +          expect(buttonNode).to.not.equal(null);
> > +
> > +          done();
> > +        });
> > +    });
> 
> This test can be written a simpler way, like:
> 
> it("should switch to displaying a check icon when the URL has been copied",
>   function() {
>     var roomEntry = mountRoomEntry();
>     var buttonNode =
> roomEntry.getDOMNode().querySelector("button.copy-link");
> 
>     TestUtils.Simulate.click(buttonNode);
> 
>     expect(buttonNode.classList.contains("checked")).eql(true);
> });

If my understanding above is correct, I could write it that way if I used forceUpdate().  Would you prefer that?

> @@ +698,5 @@
> > +        });
> > +    });
> > +
> > +    // XXX remove the .skip here (and do any necessary debugging), once
> > +    // https://github.com/facebook/react/issues/1297 is fixed.
> 
> Have you investigated the workaround mentioned in this comment?
> https://github.com/facebook/react/issues/1297#issuecomment-46623923
> 
> Here's a working example:
>
> [...]

Oohh, nice -- thanks; adopted!  I've added a forceUpdate here too.  I'm somewhat suspecting you're going to convince me that my interpretation of this stuff is wrong and I don't need to worry racing, but I'm curious to see.  I've asked in #reactjs, but no answer from the peanut gallery there just yet.  :-)
Attachment #8512132 - Attachment is obsolete: true
Attachment #8513091 - Flags: review?(nperriault)
Comment on attachment 8513091 [details] [diff] [review]
add button to copy room location to clipboard

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

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #7)
> In general, it's unusual to request review? and rather than finding a r+ or
> r-, finding an f+ instead, meaning that it's hard to interpret.  

It means it's good but not ready to land (r- might mean it's bad); I'm just trying to convey a positive message here ("review not granted" sounds a bit harsh to me). Maybe I'm getting too californian? :o

> Are you sure?  All that the click handler does is call this.setState(), and
> http://facebook.github.io/react/docs/component-api.html sez "There is no
> guarantee of synchronous operation of calls to setState".

It's just that's it's the way it's done everywhere else in test code, with no issue so far.

Note that I've locally tested your code removing all forceUpdate calls and use of callbacks and it was still working (no race, never).

> I'm somewhat suspecting you're going to convince me that my interpretation of
> this stuff is wrong and I don't need to worry racing, but I'm curious to
> see.  I've asked in #reactjs, but no answer from the peanut gallery there
> just yet.  :-)

I'd suggest we just assume that the naive approach works until we get an actual proof that it doesn't (eg. getting intermittents — which we never had in this area so far).

But I'll leave this up to you.
Attachment #8513091 - Flags: review?(nperriault) → review+
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #9)
> Comment on attachment 8513091 [details] [diff] [review]
> add button to copy room location to clipboard
> 
> Review of attachment 8513091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for
> response from comment #7)
> > In general, it's unusual to request review? and rather than finding a r+ or
> > r-, finding an f+ instead, meaning that it's hard to interpret.  
> 
> It means it's good but not ready to land (r- might mean it's bad); I'm just
> trying to convey a positive message here ("review not granted" sounds a bit
> harsh to me). Maybe I'm getting too californian? :o

I like it!  I say, keep using it, just be sure to explain what the next steps are when you do.
 
> I'd suggest we just assume that the naive approach works until we get an
> actual proof that it doesn't (eg. getting intermittents — which we never had
> in this area so far).

That seems entirely reasonable.  I'll adjust the code to match.
Comment on attachment 8513812 [details] [diff] [review]
[landed on fx-team] add button to copy room location to clipboard

Updated patch with cleaned up tests.  The suggested changes also harmonized the tests in such a way that I was able to hoist some lines into the beforeEach and DRY it out a bit.
Attachment #8513812 - Flags: review+
Attachment #8513812 - Attachment description: add button to copy room location to clipboard → [landed on fx-team] add button to copy room location to clipboard
Uh, ignore that. Slightly overzealous script on my end. That'll teach me to not set up commit messages :/
https://hg.mozilla.org/mozilla-central/rev/cf2ccaa2ec79
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Iteration: --- → 36.2
Comment on attachment 8513812 [details] [diff] [review]
[landed on fx-team] add button to copy room location to clipboard

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only
Attachment #8513812 - Flags: approval-mozilla-aurora?
Attachment #8513812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assuming the provided test coverage is sufficient. Please need-info me to request further test coverage.
Flags: qe-verify-
Flags: in-testsuite+
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.