Skip to content
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

Make showing battery on 100% configurable #306

Merged
merged 2 commits into from Jan 12, 2018

Conversation

maximbaz
Copy link
Contributor

@maximbaz maximbaz commented Jan 12, 2018

Fixes #204
Closes #232

Breaking change: by default the battery will not be shown on 100%.

Why: I don't use this module myself, but I find it very odd that by default the battery was shown on 100% (others dislike this too). This contradicts the purpose of this theme in general: to show only necessary info. Battery is almost drained is an important indicator, but battery at 100% is not an actionable thing at all, why show it then?

@denysdovhan denysdovhan added the new-feature A PR that implement feature (section, specific behavior, etc). label Jan 12, 2018
@denysdovhan
Copy link
Member

battery at 100% is not an actionable thing at all

I wouldn't agree. This indicator seems useful to me since it lets me know when I can switch off the charger. I'd prefer to keep current behavior as default and let users disable 100% indicator optionally if they want to.

What about discharged or low value for?

Copy link
Member

@denysdovhan denysdovhan left a comment

Choose a reason for hiding this comment

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

Do not change the default behavior, but add an option to disable this section when it's 100% charged.

@maximbaz
Copy link
Contributor Author

By disconnecting the charger just because the level reached 100% (and not because you have to leave) you are killing your battery, man 🙂

Regardless, I don't use this module so I don't really care about the default value, reverting the breaking change now.

@maximbaz maximbaz changed the title Make showing battery on 100% configurable and off by default Make showing battery on 100% configurable Jan 12, 2018
@denysdovhan
Copy link
Member

By disconnecting the charger just because the level reached 100% (and not because you have to leave) you are killing your battery, man 🙂

MacBook's diode indicator on the charger turns to green when charged and red when discharged (or charging). I'd prefer to reproduce this behavior in prompt too — looks consistent.

Anyway, we can change this in future breaking release, if users ask.

@denysdovhan denysdovhan merged commit 6cd9fa7 into spaceship-prompt:3.0 Jan 12, 2018
@denysdovhan denysdovhan added this to the v3.0 milestone Jan 12, 2018
@denysdovhan denysdovhan mentioned this pull request Jan 12, 2018
25 tasks
@maximbaz
Copy link
Contributor Author

maximbaz commented Jan 12, 2018

#203, #204, #296 and this all look to me like people being confused about something that they think is not actionable appearing in the prompt. Yes, part of the confusion was a missing icon / wrong font, but even so, if they thought it was actionable, the questions would have been How do I make my actionable indication of low battery more beautiful than •% ? and not What is this unactionable •% mean in my prompt? 😛

#204 and this explicitly mention that 100% being shown by default is confusing, I agree with them 😉

@nfischer
Copy link
Contributor

I find it very odd that by default the battery was shown on 100% (others dislike this too). This contradicts the purpose of this theme in general

I would agree, and I disabled the battery feature because of it.

@denysdovhan
Copy link
Member

#203 and #296 are related to the bug that has been already fixed. •% was confusing, I agree, but that was the bug.

You, @maximbaz and @nfischer, are heavy users of Spaceship, so I think I can trust your opinion on this. If you think it's clanky, I can agree to revert to 5d05c5d.

@salmanulfarzy what do you think about this situation?

@salmanulfarzy
Copy link
Member

I agree with @maximbaz and @nfischer on this.

Many users (Including me) leave charger connected after battery is completely charged. I think it'll be better to show battery only when below threshold and not when fully charged, Just like @maximbaz suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature A PR that implement feature (section, specific behavior, etc).
Development

Successfully merging this pull request may close these issues.

None yet

4 participants