halohalospecial / atom-elmjutsu

A bag of tricks for developing with Elm. (Atom package)
https://atom.io/packages/elmjutsu
MIT License
192 stars 24 forks source link

Shadow DOM #28

Closed pablomayobre closed 7 years ago

pablomayobre commented 7 years ago

Atom recently deprecated Shadow DOM, it looks like some styles in elmjutsu are still using this kind of styles, so it shows up in the deprecation cop. This should be easy to fix, I can make a PR if you need.

pablomayobre commented 7 years ago

I started to work on this in Positive07/atom-elmjutsu@f73a6773486607e9dc3b535f7874834e945e04b7 I will test and report back if this work.

PS: Atom 1.13.0 is not yet released, I'm using a beta release (atom-v1.13.0beta8-ia32) so you will most likely need to bump version by a major or minor release in order to not affect older versions of Atom, I already bumped the dependency to Atom 1.13.0.

I guess Atom 1.13 will be released soon so it's better to be prepared for it

halohalospecial commented 7 years ago

Thank you for the heads-up, @Positive07! This should be a major release. Just let me know if you need help.

halohalospecial commented 7 years ago

Looks like something else will break in Atom 1.13 :sweat_smile: https://github.com/atom/atom/issues/13189

pablomayobre commented 7 years ago

Ugh that is a problem. Can you point me to the line of code in elmjutsu where that problem will most likely be? I may be able to fix it... or that I hope haha

EDIT1: I'm using the beta version of Atom, but I haven't had problems, I'm not using elmjutsu to it's full extent though, so I haven't tested sidekick and other stuff

EDIT2: It looks like I have to rebase too, since there is a new version 😄 🎉

halohalospecial commented 7 years ago

Based on https://github.com/atom/atom/issues/13189,

this line:

event.target.getModel && helper.isElmEditor(event.target.getModel())) {

should be:

event.target.closest && helper.isElmEditor(event.target.closest('atom-text-editor'))) {

That part of the code is for this behavior (discussed in https://github.com/halohalospecial/atom-elmjutsu/issues/5):

  • "=" and other symbols such as "of' and "->" are added as suggestions
    • if the chosen suggestion is equal to what's already typed by the user (let's say the user typed "=" and it is also the primary suggestion), a newline will be inserted upon pressing enter. Doing so will save one keystroke since the user does not need to press enter twice (1 for autocomplete and 1 for newline).
    • Choosing "==" when the user typed "=" will keep the cursor on the current line

P.S. Sorry for the new version, haha. There might be more coming soon :laughing:

pablomayobre commented 7 years ago

HA! I see so that is the weird bug when I type = and it doesn't go to the next line (plus it's not like I need to press ENTER twice, it's more like I press ENTER, and suggestion appears again so I press ENTER and that becomes a loop... I actually need to press ESC first to dismiss the autocomplete dialog) like this.

I'm not sure if your proposed solution is correct I think it should be:

const editorElement = event.target.closest('atom-text-editor')
editorElement.getModel && helper.isElmEditor(editorElement.getModel())) {

Check this line in the PR to atom/line-ending-selector#35

Atom 1.13 should be around the corner, when you are done with all the releases that are comming soon I can make a new PR. So just tell me! I would be happy to help

halohalospecial commented 7 years ago

Can you kindly try it out? I haven't upgraded to Atom 1.13 yet :)

pablomayobre commented 7 years ago

I will, I'll update you tomorrow

halohalospecial commented 7 years ago

I'll probably not release anything that soon, unless there are major bugs. You can already submit a PR if you like. I'm thinking maybe we can make the changes compatible with the old versions?

pablomayobre commented 7 years ago

Will you merge it though? Since 1.13 is not released and it may brake in older versions (well not that older versions will actually get the update since the dependencies would specify atom>=1.13.0) you wont be able to continúe giving updates to 1.12

halohalospecial commented 7 years ago

Oh, I forgot about atom>=1.13.0. Thanks for reminding me! Let's just wait for the official Atom 1.13 release then. Thanks!

pablomayobre commented 7 years ago

I'll take a look soon anyway so I'll keep you updated on my findings!

I'll stop using the beta once 1.13.0 comes out... I initially installed it because 1.11 didn't have international keyboard support, and since I have been using the beta... should use the standard Atom but I'll help out with this major issues first!

halohalospecial commented 7 years ago

Thanks! :+1:

halohalospecial commented 7 years ago

Hi @Positive07, my Atom got automatically updated to 1.13.0. I already removed ::shadow from the .less files, but did not do any changes related to event.target.getModel() and it was still working as expected. The "engines" value is still "atom": ">=1.0.0 <2.0.0". You can try out the latest Elmjutsu version.

pablomayobre commented 7 years ago

Let me report some issues here

Atom 1.14 beta 0 and Atom 1.13

PS: I recommend that you use "engines : { "atom": ">=1.13.0 <2.0.0" } since earlier versions didn't support the non-shadow DOM

halohalospecial commented 7 years ago

Hi @Positive07, I still couldn't make the issues appear :-(

I did the following:

Using Atom 1.13, elmjutsu 2.18.2, Lubuntu

pablomayobre commented 7 years ago

Well I'm really sorry, I have been using an outdated Elmjutsu and I didn't notice... Weird thing is that it just recently updated to 2.17.2...

I'm checking again an reporting my findings soon.

PS: I'm not on beta anymore so I'm using standard Atom 1.13. If you want I can try 1.14beta0 but I haven't found any difference really

pablomayobre commented 7 years ago

Well I still see the deprecation call with "Pipe Selection".

Also all the errors reported happen when using Elmjutsu on an unsaved Elm file.

  • "Go to previous/next usage" hangs Atom as in an infinite loop or something
  • Sidekick is not shown
  • "Rename symbol", "Add import", "Find usages", "Go to declaration", "Go to symbol". "Eval", "Add/Remove package" does nothing

And the issue with the = is still present when using TAB, it works fine when using ENTER now which is fine 99% of the time!

halohalospecial commented 7 years ago

Hi, the latest elmjutsu version is 2.18.2. I'll check the Pipe Selections issue next time (low priority :)). Regarding pressing Tab for "=", it's not handled currently (only Enter is handled). I think the correct behavior for that should be to close the autocomplete panel. Thanks again for looking into this!

pablomayobre commented 7 years ago

Hey this has long been fixed! I'll close it