This repository has been archived by the owner. It is now read-only.

add a tooltip to the status bar display #42

Merged
merged 2 commits into from Feb 14, 2018

Conversation

Projects
None yet
5 participants
@bronson
Contributor

bronson commented Apr 7, 2017

Requirements

Description of the Change

This PR adds a tooltip to the status bar display.

image

Alternate Designs

No other designs were considered.

Benefits

Helps the user understand that this display is only referring to the highlighting of the current file.

Possible Drawbacks

A little more code and another thing to potentially leak.

Applicable Issues

Inspired by atom/encoding-selector#26.

Similar to atom/encoding-selector#45 and atom/line-ending-selector#42

@bronson

This comment has been minimized.

Contributor

bronson commented Apr 7, 2017

The failing CI doesn't appear related to this patch.

@@ -81,6 +90,8 @@ export default class GrammarStatusView {
this.grammarLink.textContent = grammarName
this.grammarLink.dataset.grammar = grammarName
this.element.style.display = ''
this.tooltip = atom.tooltips.add(this.element, {title: `File displayed using ${grammarName} highlighting`})

This comment has been minimized.

@thomasjo

thomasjo Apr 10, 2017

Member

Maybe use "grammar" instead of "highlighting" here?

This comment has been minimized.

@bronson

bronson Apr 10, 2017

Contributor

Sure. I wasn't sure how to word that... "File displayed using JavaScript grammar" vs. "File displayed using the JavaScript grammar". Both sound odd to me. :)

Well, when in doubt leave it out! Updated to use "grammar" but no "the".

@thomasjo

LGTM!

/cc @atom/feedback

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Apr 11, 2017

It seems like the tooltip doesn't add much information beyond what's already in the status bar item. I guess it's to clarify that the language is referring to the active text editor. As opposed to what though?

Almost all of the default status bar items (filename, cursor position, encoding, line ending), also refer to the current active text editor, so it seems odd for the grammar selector in particular to have a tooltip just to state that.

Thoughts? I could be totally out of touch with users' understanding here.

@bronson

This comment has been minimized.

Contributor

bronson commented Apr 11, 2017

Well, if it helps anyone, that might be worth it? Agreed, once you've used Atom for a day or two, this won't tell you anything you don't already know.

That said, I find it odd to scan the mouse cursor across the status bar's tooltips and stumble on one or two dead items. Feels like they're unfinished.

This one and atom/line-ending-selector#42 are the only holdouts... If they're merged, the entire status bar will respond the same way to hovers.

Consistency gives me comfort. :)

@bronson

This comment has been minimized.

Contributor

bronson commented Apr 20, 2017

Now that atom/line-ending-selector#42 is in, this is the only item in the status bar without a tooltip. FWIW.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Oct 25, 2017

I'm not opposed to this but I don't like the use of highlighting. To me that implies that the language package is the one doing the highlighting when it's not. How about:

File uses the language grammar

which isn't that clear either, but I'd like to avoid the implication that language packages also syntax highlight.

Unrelated: the encoding tooltip has a This in the beginning while this PR and line-ending-selector don't.

@bronson

This comment has been minimized.

Contributor

bronson commented Oct 26, 2017

@50Wliu sure, happy to make the wording change. And, agreed, omitting This is probably best.

File uses the Plain Text grammar

@bronson

This comment has been minimized.

Contributor

bronson commented Jan 3, 2018

Updated. Huge thanks @maxbrunsfeld for demonstrating tests in atom/line-ending-selector#48

@ungb

This comment has been minimized.

Contributor

ungb commented Feb 14, 2018

Thanks for your perseverance on this pr @bronson and adding test! I tested everything out and LGTM.

I'll get this on Atom master shortly after merging.

@ungb ungb merged commit b1e6e51 into atom:master Feb 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.