Add copy shot to clipboard #4917
Conversation
| document.addEventListener("addon-present", (e) => { | ||
| if (e.detail) { | ||
| const capabilities = JSON.parse(e.detail); | ||
| if (capabilities["copy-to-clipboard"]) { |
There was a problem hiding this comment.
This prevents the copy button from being displayed to older versions of the addon.
server/src/pages/shot/view.js
Outdated
| style={{ height: "auto", width: Math.floor(clip.image.dimensions.x) + "px", maxWidth: "100%" }} | ||
| ref={clipImage => this.clipImage = clipImage} | ||
| src={clip.image.url} | ||
| alt={clip.image.text} />; |
There was a problem hiding this comment.
Only formatting changes.
| <Editor clip={clip} pngToJpegCutoff={this.props.pngToJpegCutoff} onCancelEdit={this.onCancelEdit.bind(this)} onClickSave={this.onClickSave.bind(this)}></Editor> | ||
| <Editor clip={clip} pngToJpegCutoff={this.props.pngToJpegCutoff} | ||
| onCancelEdit={this.onCancelEdit.bind(this)} | ||
| onClickSave={this.onClickSave.bind(this)}></Editor> |
There was a problem hiding this comment.
Formatting changes.
| onClick={this.onClickEdit.bind(this)} | ||
| onMouseOver={this.onMouseOverHighlight.bind(this)} | ||
| onMouseOut={this.onMouseOutHighlight.bind(this)}></div> | ||
| : null; |
There was a problem hiding this comment.
Formatting changes.
server/src/pages/shot/view.js
Outdated
| <span className="btn-text">Download</span> | ||
| </Localized> | ||
| </button> | ||
| </Localized></div>; |
There was a problem hiding this comment.
Formatting changes. ☝️
New button. 👇
chenba
force-pushed
the
4776-copy-shot-img
branch
from
3076608
to
528f9c3
Compare
| <span>Download</span> | ||
| </Localized> | ||
| </button> | ||
| </Localized></div>; |
There was a problem hiding this comment.
Formatting changes. ☝️
New button. 👇
| &:active { | ||
| background-color: $light-active; | ||
| } | ||
| } |
There was a problem hiding this comment.
Leftover styles from the report shot flag that was moved to the footer in #4687. The background image file is also removed in this PR.
|
@chenba Looks like this needs a rebase |
chenba
force-pushed
the
4776-copy-shot-img
branch
from
528f9c3
to
2654561
Compare
There was a problem hiding this comment.
Looking at the code...I just don't know about all this. This copy button idea seems weird to me, and the implementation seems rather complex.
We are relying on the addon to actually do the copying, but don't want to show an unusable button if the addon's missing or an old version...which requires us to ping the addon first to see if it has the copy powers (messages sent as DOM events, which are slow). I'm not sure, but it seems likely to have some effect on page load time.
We're also showing website copy button click errors as desktop/Firefox notifications, which is odd, and breaks the permission model used by every other website (show a permission doorhanger before showing desktop notifications). Desktop notifications for Screenshots are fired when the addon shot creation flow fails, not when the website fails. Doing this also entails sending another message between website and addon :-(
This seems like relatively a lot of complexity for questionable value: seems likely that users who want to copy the image will know to right-click and choose "Copy Image" from the context menu. @johngruen What do you think, do we really need this copy button?
+1 And good point about right click to copy. |
|
@chenba Yeah, sorry I didn't think to raise these points before you did all the work in this PR :-P |
|
Talked about this in the morning meeting, and @johngruen wants to land it and see if people like it. Merging 👍 |
| const copyButton = <div className="copy-img-button" hidden={!this.state.canCopy}> | ||
| <Localized id="shotPageCopyButton"> | ||
| <button className="nav-button icon-copy transparent copy" | ||
| title="Copy image to clipboard" |
There was a problem hiding this comment.
Looks like this string wasn't exposed for localization. We'll extract is in #4935
Fixes #4776