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.

Sign up for GitHub

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

Jump to bottom

now detecting accessKey modifiers based on userAgent #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

now detecting accessKey modifiers based on userAgent #1

wants to merge 2 commits into from

Conversation

jkissel
Copy link

@jkissel jkissel commented Jan 8, 2017

No description provided.

nipafx
nipafx reviewed Jan 8, 2017
View reviewed changes
Copy link
Member

@nipafx nipafx left a comment

Choose a reason for hiding this comment

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

Found two things to complain about...

By the way, it's easier for everybody involved if you create a new branch for something you want to open a pull request on.

src/workflowyfx.js
function accessKeyText(accessKey) {
return accessKeyModifier() + ` + ` + accessKey
function accessKeyHtml(accessKey) {
return (modsHTML + accessKey)
Copy link
Member

Choose a reason for hiding this comment

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

Compile error. 😜

src/workflowyfx.js
var modsHtml;
var mods = accessKeyModifiers(navigator.userAgent)
if (mods) {
modsHtml = `<kbd>${mods.join('</kbd> + <kbd>')}</kbd> + `
Copy link
Member

Choose a reason for hiding this comment

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

The kbd made me curious. I thought you might've picked them up from the original source, so I went looking there. Look what I found:

<td class="commandDescription">
	<span class="showOnWindows showOnLinux">Alt</span>
	<span class="showOnChromeOS">Control</span>
	<span class="hideOnWindows hideOnLinux hideOnChromeOS">⌘</span> + Shift + Up/Down
</td>

I'd like to stay close to original so unless you think this is a terrible idea, I'd want to go that way. Would eliminate any browser sniffing code from the extension (for now), which is not bad either. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nipafx nipafx nipafx left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jkissel @nipafx