search

StreamMeDev / hermes

A react message input with autocomplete and other handy features
ISC License
6 stars 0 forks source link

remove ctrl-c shortcut that overlaps windows copy functionality #16

Closed Jeddf closed 4 years ago

Jeddf commented 6 years ago

resolve #14

wesleytodd commented 6 years ago

After looking at this, it would be super trivial to keep the feature but turn it off based on a prop or environment detection. I think it is overkill to remove it for all users when it is a better experience to implement what platform specific users would expect.

Jeddf commented 6 years ago

What environment specifically would you say the current shortcuts target? I could add a flag like 'shortcutSet' that has environments enumerated that defaults to none and has just has one available option at first?

wesleytodd commented 6 years ago

The current shortcuts target OSx and a bit of nix (dont think many people will care about nix though). TBH I think it is reasonable for hermes to just make a decision about its defaults but have the "dumb" underlying components be un-opinionated. So like have that file turn ctrl-c to a noop in windows, and esc into a noop on osx/*nix.

Jeddf commented 6 years ago

Looked around for libraries that could do that OS detection and this seemed like the most likely candidate: https://github.com/bestiejs/platform.js/. Seems kinda heavy though and I'm not sure I agree with the idea that an autocomplete box should be worrying about platform distinct keyboard shortcuts anyway.

As far as StreamMe goes we wanted to ship and weren't looking to have keyboard shortcuts but I want to respect this library being OSS and having it's own community. Taking a hard turn in terms of direction just because StreamMe wants to do something ain't right and isn't really necessary. We've forked internally for now, with no keyboard shortcut overrides and using input/textarea instead of the contenteditable.

I'd spent about a week grinding away at little edge cases around nbsp and text selection and the inconsistencies in behaviour between browsers. Will hopefully come back soonish and bring the fruits of that work to the open source portion here. Using contenteditable in the long run would obviously be preferable as it allows all the good stuff with styling the content as HTML.

Jeddf commented 6 years ago

Also! I realize I'd closed a previous unmerged but approved PR from you @wesleytodd, sorry about that, I'd fully intended to have those changes folded in with my work but you had a line where you subbed in nbsp's for all spaces and that caused the words to not wrap properly. That week I mentioned above working on nbsp and contenteditable did not end with anything I was happy offering up, hence this being a little anti-climactic.

wesleytodd commented 6 years ago

I mean if the library doesn't serve the needs than it isn't good. So make the necessary changes. My comments were more to make sure they were not being short-sighted for expediency sake. In this case, I fully agree that adding that lib is WAY overkill. Killing the ctrl-c binding seems like the right solution in that regard.

That being said, forking internally also seems like a bad idea. The whole point of publishing this was that we could have external users or bot authors, at some point, using the same code in use on the channel page. So a fork doesn't even solve that.

wesleytodd commented 6 years ago

with no keyboard shortcut overrides and using input/textarea instead of the contenteditable.

So you dropped the highlighting? I am pretty confused because there were very few known issues with the implementation here other than that spaces one you posted about, for which I think the fix was really simple. Maybe post more issues and see if I can give any other direction?

Jeddf commented 6 years ago

Yeh absolutely, there were issues around selecting and pasting text also, I'll get 'em into issues here soon.