search

artursapek / mondrian

Web-based vector graphics editor
MIT License
1.24k stars 104 forks source link

Map the ctrl key as cmd if the user is on windows. #36

Closed Fishrock123 closed 10 years ago

Fishrock123 commented 10 years ago

Fixes #1.

artursapek commented 10 years ago

More important is changing the cmd modifier symbol in the menus to say "ctrl".

I would do this with a pseudo-element, eg give the body a class that specifies what OS the use is on and that class determines the content of the cmd/ctrl modifier. Make sense?

Fishrock123 commented 10 years ago

I wouldn't consider that more important; most important is that it now works for windows, even if slightly hacky. (The entire unless ~ thing can get pretty weird to visualize) I'm pretty sure most windows people would naturally try to use ctrl with these hotkeys.

That does make sense though. Not 100% sure how I'd do it with CSS off the top of my head, but I'll have a jab at it.

artursapek commented 10 years ago

No, sorry. I didn't mean that. I meant it's more important than the style question of if not over if !. Of course it has to work before we change the icons in the menu.

This looks great though. I wouldn't call it hacky. Great job. I'll test this out tonight.

artursapek commented 10 years ago

Have you been able to test this on a Windows machine? I have none at my disposal and it doesn't seem to work when I test it in Windows on Browserstack, but it might be a problem with Browserstack.

Fishrock123 commented 10 years ago

I did test it on Windows 8.

I'll host this revision on my site later and make sure it works for the OP.

Edit: (I would do this now but npm is crapping out on me.)

Fishrock123 commented 10 years ago

Protip: do not rely on MouseEvent.metaKey.

Fishrock123 commented 10 years ago

New commits confirmed working in Windows 8 & in OS X.

@artursapek LGTY?

artursapek commented 10 years ago

I deployed this to staging and I'll test it on a Windows machine while I'm at work tomorrow.

Fishrock123 commented 10 years ago

@artursapek Were the latest commits deployed to staging?

Fishrock123 commented 10 years ago

@artursapek I know you have mainly moved on to other things, but do you think this could be pulled?

artursapek commented 10 years ago

Deployed. Can you verify?

Fishrock123 commented 10 years ago

Works for me. (Windows 8, Chrome)

artursapek commented 10 years ago

Great!

icarito commented 7 years ago

Perhaps this should be the default, and the conditional should be to use cmd for OSX only? Currently shortcuts don't work for me on GNU/Linux.