microsoft / TypeScript-Sublime-Plugin

IO wrapper around TypeScript language services, allowing for easy consumption by editor plugins
Apache License 2.0
1.72k stars 237 forks source link

Info and error popup content copyable to clipboard #724

Closed kylebebak closed 4 years ago

kylebebak commented 5 years ago

This PR contains two changes.

The first is a simple usability win; info and error popup content is copyable to the clipboard, via a popup link/button below the content.

HTML tags are stripped from the content. Line breaks and general formatting are conserved. When the user presses the copy link, a status_message is printed confirming the text was copied.

Screen Shot 2019-07-08 at 5 16 36 PM

This is the text copied to the clipboard:

Screen Shot 2019-07-08 at 5 17 25 PM

The other change, which is not related to popup content at all, but which IMO is a big improvement as well, is getting rid of the snippets that are bundled with this plugin.

These snippets are all based on, or are identical to, the snippets that ship with Sublime Text's default JavaScript package, and which pretty much everyone agrees were a mistake.

Just to give one egregious example, the "improved" for loop:

for (var i = Things.length - 1; i >= 0; i--) {
    Things[i]
}

There are some good comments in that thread, especially from @Thom1729 , regarding why bundling snippets with a package is a bad idea, even if said snippets aren't as wacky as the one above.

Maybe someone thought copying Sublime's default JS snippets to this package would be comforting for existing Sublime users, but it was copying a misfeature, and just creates noise and makes autocomplete less useful for everyone.

I'd be glad to separate this into its own PR if it's necessary, but it seems like such a clear usability win that I figured the sooner it gets merged the better.

orta commented 4 years ago

OK, so - the snippets feels a bit controversial?

I'm totally happy to get the copy button in, I copy this stuff all the time 👍

kylebebak commented 4 years ago

@orta

Brought the snippets back!

Fortunately, there's an upcoming ST build that's going to let us ignore default snippets using an ignored_snippets setting.

I also got the copy error functionality working again, it had broken because this PR was old and when I merged all the new commits into it there were a bunch of merge conflicts.

The sooner we get it merged the better!

kylebebak commented 4 years ago

@orta @DanielRosenwasser

Just bumping, because I think this is ready to merge =)

orta commented 4 years ago

I will take a look at this but I need to get back home to NYC from my vacation to test this (I broke the last sublime text update, so I want to at least get my dev env set up to validate master and PRs are working as expected)

so thanks for the update! (and if you don't hear from me by next Friday, ping this PR)

kylebebak commented 4 years ago

@orta

Just bumping to see if you can eyes on this and we can get it merged. Thank you!

orta commented 4 years ago

Alright, great timing on your ping. I got back yesterday and gave it a run through just now

Screen Shot 2020-01-15 at 11 30 22 AM

Looks good to me 👍

kylebebak commented 4 years ago

Thanks a bunch!!!