1074674 - 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

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Expected UX - Hover

Comment 1

[Mark Banner (:standard8)]

Attachment: Expected UX - Clicked

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Updated hover animation link to one that works.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

(Commenting on 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

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

https://github.com/dmose/gecko-dev/compare/copy-room-link?expand=1 has a WIP branch.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: add button to copy room location to clipboard

Comment 6

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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/[e-mail]    (content/shared/img/[e-mail])
> +  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);
});

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

(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/[e-mail]    (content/shared/img/[e-mail])
> > +  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.  :-)

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: add button to copy room location to clipboard

Comment 9

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(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 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: [landed on fx-team] add button to copy room location to clipboard

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/cf2ccaa2ec79

Comment 14

[Chris Kitching [:ckitching]]

https://tbpl.mozilla.org/?tree=Try&rev=cf2ccaa2ec79

Comment 15

[Chris Kitching [:ckitching]]

Uh, ignore that. Slightly overzealous script on my end. That'll teach me to not set up commit messages :/

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf2ccaa2ec79

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f634a1289875

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 19

[u279076]

Assuming the provided test coverage is sufficient. Please need-info me to request further test coverage.