Skip to content
This repository has been archived by the owner on Oct 19, 2018. It is now read-only.

add a tooltip to the status bar display #42

Merged
merged 2 commits into from
Feb 14, 2018
Merged

Conversation

bronson
Copy link
Contributor

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

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`})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link

@thomasjo thomasjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

/cc @atom/feedback

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

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

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.

@winstliu
Copy link
Contributor

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

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

bronson commented Jan 3, 2018

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

@ungb
Copy link
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants