Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed incorrect instructions: "Press ⌘ to copy" #20602

Closed
wants to merge 4 commits into from

Conversation

tjkohli
Copy link
Contributor

@tjkohli tjkohli commented Aug 31, 2016

Changed to "Press ⌘C to copy"

It needed to be "Press ⌘C to copy"
@wolfy1339
Copy link
Contributor

According to http://www.fileformat.info/info/unicode/char/2318C/index.htm this PR would be wrong

@tjkohli
Copy link
Contributor Author

tjkohli commented Sep 7, 2016

@wolfy1339 Have you tried the code in your browser? The last C is the letter "C" and not part of the character. u2318 renders the command icon (⌘)

@wolfy1339
Copy link
Contributor

Yes I have, but It can be mis-interpreted as another Unicode glyph. So it would be better to use string concatenation to avoid any potential bugs when using any other font than the ones in default Bootstrap

@Kovah
Copy link
Contributor

Kovah commented Sep 8, 2016

Why not simply using \u2318 + C = ⌘ + C?

@tjkohli
Copy link
Contributor Author

tjkohli commented Sep 8, 2016

@Kovah I do not recommend that as Apple stylizes shortcuts without a '+' sign, and adding a '+' could possibly confuse users. There is no '+' action between the ⌘ and the C, so ⌘C makes the most sense.

Updated copy command to concatenated string to prevent potential issues
@@ -73,7 +73,7 @@
})

clipboard.on('error', function (e) {
var fallbackMsg = /Mac/i.test(navigator.userAgent) ? 'Press \u2318 to copy' : 'Press Ctrl-C to copy'
var fallbackMsg = /Mac/i.test(navigator.userAgent) ? 'Press \u2318' + 'C to copy' : 'Press Ctrl-C to copy'

Choose a reason for hiding this comment

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

Unexpected var, use let or const instead no-var
Unexpected string concatenation of literals no-useless-concat

@tjkohli
Copy link
Contributor Author

tjkohli commented Sep 8, 2016

I updated it to concatenate the two pieces. Should this work now?

@tomlutzenberger
Copy link
Contributor

I'd recommend 'Press \u2318 + C to copy'(remove those single-quotes).

@tjkohli
Copy link
Contributor Author

tjkohli commented Sep 8, 2016

@tomlutzenberger I don't recommend that, at least for Mac users, who this PR applies to.

I do not recommend that as Apple stylizes shortcuts without a '+' sign, and adding a '+' could possibly confuse users. There is no '+' action between the ⌘ and the C, so ⌘C makes the most sense.

screen shot 2016-09-08 at 10 32 54 am

@tomlutzenberger
Copy link
Contributor

Hm okay...
In that case I would do the following:

      const modifierKey = /Mac/i.test(navigator.userAgent) ? '\u2318' : 'Ctrl-';
      let fallbackMsg = 'Press ' + modifierKey + 'C to copy';

That way you dodge unnecessary string concatenation AND prevent duplicate code/text.
Note that I also changed var to let.

@@ -73,7 +73,8 @@
})

clipboard.on('error', function (e) {
var fallbackMsg = /Mac/i.test(navigator.userAgent) ? 'Press \u2318 to copy' : 'Press Ctrl-C to copy'
const modifierKey = /Mac/i.test(navigator.userAgent) ? '\u2318' : 'Ctrl-';
let fallbackMsg = 'Press ' + modifierKey + 'C to copy';

Choose a reason for hiding this comment

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

Unexpected string concatenation prefer-template
Extra semicolon semi

@tomlutzenberger
Copy link
Contributor

Oh yes, I forgot. They have this "no-semicolon-on-line-endings"-Rule.
After removing those, it should be fine @tjkohli

@tjkohli
Copy link
Contributor Author

tjkohli commented Sep 8, 2016

@tomlutzenberger Done. Thanks dude!

@tomlutzenberger
Copy link
Contributor

Why do the checks still fail?
Is there a possibility to restart them?

//cc @cvrebert

@bardiharborow
Copy link
Member

@tjkohli could you rebase and squash these commits, and force push the branch again. This will also trigger Travis to rebuild this PR.

@mdo mdo added this to the v4.0.0-alpha.6 milestone Oct 29, 2016
@mdo mdo mentioned this pull request Nov 28, 2016
mdo added a commit that referenced this pull request Nov 28, 2016
* Fixed incorrect instruction: "Press ⌘ to copy"

It needed to be "Press ⌘C to copy"

* Updated to concatenate copy string

Updated copy command to concatenated string to prevent potential issues

* Updated the way the copy string concatenates

As per excellent suggestion by @tomlutzenberger

* Removed semicolon line endings

;P

* var not const or let
@mdo mdo mentioned this pull request Nov 28, 2016
@mdo
Copy link
Member

mdo commented Nov 28, 2016

Merged this with #21236, so closing. Thanks!

@mdo mdo closed this Nov 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants