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

Changes default max width of HTML popup view from 1500 to 1024; adds error_color and quick_info_popup_max_width settings #715

Closed kylebebak closed 5 years ago

kylebebak commented 5 years ago

The smallest screen resolution for laptops with a significant number of users worldwide is 1024×768 (xga – standard).

This PR lowers the max_width of the quick info popup from 1500 to 1024. Popups with a width of 1500 aren't properly visible on many laptops, including my 13 inch MBP, which has a width of 1280.

It also adds error_color and quick_info_popup_max_width settings, which can be overridden in Packages/User/TypeScript.sublime-settings.

The error_color setting would properly solve issues like #489 .

Here's a screenshot to illustrate the popup width issue:

Screen Shot 2019-04-28 at 10 22 08 AM

And here's a screenshot to show type errors underlined in red, using "error_color": "region.redish". The possible error colors are limited by the Sublime Text API. The method used in this PR is the same as the method used by the Bracket Highlighter plugin. See Clearing Up Color Misconceptions for a detailed explanation.

Screen Shot 2019-04-30 at 8 07 32 AM
msftclas commented 5 years ago

CLA assistant check
All CLA requirements met.

kylebebak commented 5 years ago

@DanielRosenwasser

Just bumping and hoping someone can review this and merge it soon. It's nothing special, just some real low-hanging fruit.

Thanks!

DanielRosenwasser commented 5 years ago

Hey @kylebebak, looks like there are merge conflicts Can you fix those up?

Also, instead of a fixed width, why not make the tooltips dependent on the viewport/layout width? Maybe just make it the layout width with a margin of 1em (view.layout_extent()[0] - 2 * view.em_width()).

See more at https://www.sublimetext.com/docs/3/api_reference.html#sublime.View

I would hold off on adding a configuration option for width if that's okay.

kylebebak commented 5 years ago

@DanielRosenwasser Hey, thanks for the suggestion, this is a great idea and a definite improvement!

I didn't know about this part of ST's API, so thanks for that too.

I'll fix the merge conflicts and push again in a while.

kylebebak commented 5 years ago

@DanielRosenwasser

Hey again! Merge conflicts are now resolved.

Also, a few updates regarding error popup width...

ST has two view methods to get dimensions, viewport_extent and layout_extent, but neither of these returns the full width of the screen.

The gutters, the sidebar, the file preview column on the right... These aren't included in the widths returned by viewport_extent and layout_extent. I looked through the API, and it looks like these are the only methods for getting screen dimensions, so there's no way to use the ST API to get the width of the user's screen.

I understand the reluctance to add settings, but relying on view.layout_extent or view.viewport_extent means a lot of screen width is unused. On my machine, it's around 850 pixels instead of 1280, about a third of the screen. There are screenshots below showing what it looks like.

I would definitely like the full width of the screen for error popups, because sometimes error messages are long, and the more I can see without scrolling the better. My vote is to keep the quick_info_popup_max_width setting. The commit I just pushed falls back to view.viewport_extent()[0] if quick_info_popup_max_width isn't set. Thanks for that suggestion!

Please let me know what you think. Thanks!

Using view.layout_extent()[0]:

Screen Shot 2019-05-23 at 2 27 10 PM

Using view.viewport_extent()[0] is a little wider, but still not wide enough for my taste.

Screen Shot 2019-05-23 at 2 26 26 PM
kylebebak commented 5 years ago

@DanielRosenwasser

Is there some reason this can't be merged?

kylebebak commented 5 years ago

@DanielRosenwasser

Just bumping this again. I have another PR that allows users to copy all type and error messages from the tooltip / popup, but I don't want to submit it until this one is closed or merged...

Also, and this is sort of unrelated, but I'd like to volunteer to help maintain this plugin. It could clearly use more hands. I've written lots of ST plugins, some with with large code bases and complex behavior, e.g. Requester.

I use TypeScript pretty much every day at work, love the language, and am also committed to using Sublime Text long term.

DanielRosenwasser commented 5 years ago

Hey @kylebebak, thanks for being patient. I think we'd welcome more contributions, and thinking about it more, 1024 as a default can always be adjusted (or removed altogether).

DanielRosenwasser commented 5 years ago

Thanks so much for the PR!

kylebebak commented 5 years ago

@DanielRosenwasser

No, thanks for merging it!

I'll submit the other PR I mentioned (the one that makes it possible to copy info from the type or error popups) in a bit!