Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

add a tooltip to the encoding status view #45

Merged
merged 4 commits into from Apr 7, 2017

Conversation

bronson
Copy link
Contributor

@bronson bronson commented Apr 6, 2017

Requirements

Description of the Change

This PR adds a tooltip to the status bar indicator.

Alternate Designs

No other designs were considered. Happy to make changes.

Benefits

Unfamiliar users will be reassured that, yes, this indicator does refer to the file being edited.

Also, most status bar items have tooltips. This offers a bit of reassuring consistency.

Possible Drawbacks

Maybe I'll leak the tooltip if I missed a place where it needs to be disposed?

Applicable Issues

#26

@bronson
Copy link
Contributor Author

bronson commented Apr 7, 2017

Right now the wording is: File displayed using UTF-8 encoding.

if(this.toolTip) {
this.toolTip.dispose()
}
this.toolTip = atom.tooltips.add(this.encodingLink, {title: 'File displayed using ' + encodingLabel.list + ' encoding'})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we generally use this.tooltip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's better. Changed.

@50Wliu 50Wliu requested a review from damieng April 7, 2017 00:27
@@ -64,6 +68,11 @@ export default class EncodingStatusView {
this.encodingLink.textContent = encodingLabel.status
this.encodingLink.dataset.encoding = editorEncoding
this.element.style.display = ''

if(this.tooltip) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a space between if and (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. And installed the linter-js-standard package.

if(this.tooltip) {
this.tooltip.dispose()
}
this.tooltip = atom.tooltips.add(this.encodingLink, {title: 'File displayed using ' + encodingLabel.list + ' encoding'})
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if 'displayed' is the right word. It's parsed with the encoding so if you change encoding and save it you can lose information. Displayed sounds like it's completely passive. Also consider using string interpolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point. Changed to This file uses UTF-8 encoding.

I wasn't sure I wanted to be the first to use interpolation in this package... but it works fine. :)

@damieng damieng merged commit 63b3a6e into atom:master Apr 7, 2017
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.

None yet

3 participants