Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Fixes #5146 - Copy string on myshots shows blank #5153

Merged
merged 1 commit into from
Nov 8, 2018

Conversation

punamdahiya
Copy link
Contributor

No description provided.

@@ -155,10 +155,10 @@ class ShareButtonPanel extends React.Component {
value={ this.props.shot.viewUrl }
onClick={ this.onClickInputField.bind(this) }
onChange={ function() {} /* react gives a warning otherwise */ } />
<Localized id={ this.state.copy === "copy" ? "shotPageCopy" : "shotPageCopied" }>
<Localized id={ this.state.copy === "copy" ? "shotPageCopyText" : "shotPageCopied" }>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. What was "shotPageCopy" before this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flodolo that was for exact same text "Copy" but got removed in a PR that landed for new copy button on shots.
Bringing it back with new id as we still need it on my shots page or can we still use the same id in this case?

@@ -100,6 +100,7 @@ shotPageShareButton =
.title = Share
shotPageCopyButton =
.title = Copy image to clipboard
shotPageCopyText = Copy
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English is unclear (noun or verb?), I'd prefer a different ID to avoid confusion, e.g. shotPageCopyActionLabel (since shotPageCopyButton is already in use)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a verb, will update with suggested ID

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

@flodolo flodolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WFM from a l10n point of view

@jaredhirsch jaredhirsch merged commit 7c58bb1 into mozilla-services:master Nov 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants