InfiniteAmmoInc / Yarn

934 stars 93 forks source link

Update Ace library #73

Closed Kaosumaru closed 5 years ago

Kaosumaru commented 6 years ago

Hi, it seems that older version of ace library is bit buggy - it seems to block right alt+[a/z/l] keys (commonly used to input letters extending latin alphabet in some languages). I haven't pinpointed the exact issue, but updating ace to newest version seems to fix it. Since electron branch is in active development now, I'm setting it as target.

julsam commented 6 years ago

I'm using an US International keymapping on mac, and it does indeed some weird things, i didn't even realize 😱

Nice catch and good to know newest version of ace fixes it.

blurymind commented 5 years ago

This is a nice fix. Thank you :)

desplesda commented 5 years ago

Should this be updated to use ace as a dependency via packages.json?

blurymind commented 5 years ago

right now it is just included with Yarn in the libs folder, I need to refactor the code a little bit so it uses require to include it from node_modules instead. That way we will be able to use npm to update it in the future from https://www.npmjs.com/package/ace-builds .. So for yarn-electron I am gradually moving external dependencies to package.json and removing them from the lib folder, but haven't gotten to ace editor yet. :) In any case pulls to do that are welcome.

This pull just overwrites the one in the libs folder

blurymind commented 5 years ago

Ok, lets merge this like it is for now- as it doesnt cause any regressions. We can move it to node_modules in another pull

blurymind commented 5 years ago

@desplesda I just merged an update to allow updating ace and its modules via npm https://github.com/InfiniteAmmoInc/Yarn/commit/81b7326659612c403c5f5d50caf7a6d51a739d3f

There are still a few other libraries that need to be migrated, but lets take it slowly with that kind of re-factoring as it needs testing for regressions