Remove atom-space-pen-views and custom elements #1032
Conversation
|
@ungb: some things in this pull request that might be worth testing 💭
|
lib/tree-view.coffee
Outdated
| isRecursive = e.altKey or false | ||
| @selectEntry(entry) | ||
| if entry instanceof DirectoryView | ||
| if entry.classList.contains('directory') |
There was a problem hiding this comment.
This raises an exception if the user clicks on the tree-view's background, rather than a specific entry:
tree-view.coffee [sm]:225
Uncaught TypeError: Cannot read property 'classList' of null
module.exports.TreeView.entryClicked @ tree-view.coffee [sm]:225
(anonymous function) @ tree-view.coffee [sm]:100
There was a problem hiding this comment.
Thank you so much for the heads-up! Fixed in 31c472d. 👍
There was a problem hiding this comment.
Haha, my pleasure! I'm running latest changes through Mocha over and again, so it's easy to spot when something breaks. =)
|
|
|
Might be worth shimming |
|
Thanks again for the amazing feedback and for providing a fix on the seti-ui package, @Alhadis. Your help is really appreciated! ✨ I am on the fence about shimming because it seems like it could be non-trivial to provide a compatible interface. In particular, there are a lot of jQuery methods that might be called from third-party packages and re-implementing them by using DOM primitives could take long, other than being potentially inaccurate. An alternative might be to use a |
|
Oh, I was only talking about shimming the tree-view's To be honest, breakage will probably get packages fixed quicker than deprecation notices. That is, if the speed at which I'm probably biased, I just want jQuery to go away as quickly as possible. |
Yeah, my main concern with that is that we would also need to wrap the DOM event inside a jQuery event object and give users a way of providing a selector as the second parameter of the
Agreed. I also think that the number of packages affected by this is actually low, so it might be worth seeing what happens on beta and then decide whether a shim is worth it or not. 👍 |
|
Oh right, I completely forgot about the |
lib/tree-view.coffee
Outdated
| directory.collapse(isRecursive) | ||
| @selectEntry(directory) | ||
|
|
||
| openSelectedEntry: (options={}, expandDirectory=false) -> | ||
| selectedEntry = @selectedEntry() | ||
| if selectedEntry instanceof DirectoryView | ||
| if selectedEntry.classList.contains('directory') |
There was a problem hiding this comment.
This will break when nothing is selected, because selectedEntry is null.
Happened after I hit Enter with focus on the tree-view by mistake.
|
Looks good to me, but I'm also seeing the same issue @Alhadis is with seti-ui, I wonder if this will cause other breakages for other packages. We can maybe put it out on beta and use bugsnag to capture more exceptions. |
This pull request replaces atom-space-pen-views with manual DOM manipulation. It also changes
DirectoryViewandFileViewso that they don't register themselves as custom elements, but are simple JavaScript objects managing a DOM element. Feature-wise nothing should change, but this will help remove jQuery from Atom./cc: @ungb for 👀