8000 Info and error popup content copyable to clipboard by kylebebak · Pull Request #724 · microsoft/TypeScript-Sublime-Plugin · GitHub
[go: up one dir, main page]

Skip to content

Info and error popup content copyable to clipboard #724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 15, 2020

Conversation

kylebebak
Copy link
Contributor

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
Copy link
Contributor
orta commented Sep 11, 2019

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
Copy link
Contributor Author

@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 kylebebak changed the title Info and error popup content copyable to clipboard; removes default snippets Info and error popup content copyable to clipboard Dec 20, 2019
@kylebebak
Copy link
Contributor Author

@orta @DanielRosenwasser

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

@orta
Copy link
Contributor
orta commented Jan 6, 2020

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
Copy link
Contributor Author

@orta

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

@orta
Copy link
Contributor
orta commented Jan 15, 2020

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 👍

@orta orta merged commit c79e0c9 into microsoft:master Jan 15, 2020
@kylebebak
Copy link
Contributor Author

Thanks a bunch!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0