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

Ctrl+S doesn't work on Windows #108

Closed
csagonas opened this issue Oct 13, 2015 · 6 comments
Closed

Ctrl+S doesn't work on Windows #108

csagonas opened this issue Oct 13, 2015 · 6 comments
Assignees
Labels

Comments

@csagonas
Copy link

Notes by @jabooth:

Unfortunately on Chrome Windows, Ctrl + S brings up the save webpage dialog, so we can't actually save with the keyboard. Maybe we should just do S? How does something like gmail get around these kind of cross-platform issues?

@jabooth jabooth added the bug label Oct 17, 2015
@lirsacc
Copy link
Contributor

lirsacc commented Oct 17, 2015

We use ctrl to avoid unwanted save as it is reflected on the backend. I feel like just S is too prone to accidental saves.

I think we don't use preventDefault on this one though, so it may be a simple fix, but I don't have access to a windows laptop to confirm.

EDIT: See branch https://github.com/menpo/landmarker.io/tree/keyboard-issues where I moved it to the keydown handler and successfully bypass the dialog on OS X, if anyone can confirm that it works on win we can go ahead and merge

@lirsacc lirsacc self-assigned this Oct 17, 2015
@jabooth
Copy link
Member

jabooth commented Oct 20, 2015

@lirsacc I can try this on Windows tonight.

@jabooth
Copy link
Member

jabooth commented Oct 20, 2015

@lirsacc confirmed, that works as a fix.

Can you make a PR for the branch and just reference this issue? Now we are incrementally I'd rather have individual PRs to point to for history if possible.

Once that is merged, we can close this issue, and do a patch release.

@lirsacc
Copy link
Contributor

lirsacc commented Oct 21, 2015

Linked to #110 as the refactor encompassed both. Unless it's urgent I'd wait for both to be cleared.

@jabooth
Copy link
Member

jabooth commented Oct 21, 2015

No problem, we'll wait for #110 to get resolved first.

@jabooth
Copy link
Member

jabooth commented Oct 28, 2015

Solved by #122

@jabooth jabooth closed this as completed Oct 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants