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

Change weatherforecast and currentweather unit display #1503

Closed
fwitte opened this issue Jan 3, 2019 · 4 comments
Closed

Change weatherforecast and currentweather unit display #1503

fwitte opened this issue Jan 3, 2019 · 4 comments

Comments

@fwitte
Copy link
Contributor

fwitte commented Jan 3, 2019

Hi everybody,
I just started building/configuring my own MM and found a bug using "default" for the units. The degree symbol should not be displayed. It is degree Celsius and degree Fahrenheit, but not degree Kelvin.

weatherforecast
https://github.com/MichMich/MagicMirror/blob/de57daa3cd881ce1a14b88307bf61e8109879c81/modules/default/weatherforecast/weatherforecast.js#L145-L158

currentweather
https://github.com/MichMich/MagicMirror/blob/de57daa3cd881ce1a14b88307bf61e8109879c81/modules/default/currentweather/currentweather.js#L200-L213

https://github.com/MichMich/MagicMirror/blob/de57daa3cd881ce1a14b88307bf61e8109879c81/modules/default/currentweather/currentweather.js#L221

I suggest changing the display for both modules like this:

var degreeLabel = "";
if(this.config.scale) {
	switch(this.config.units) {
	case "metric":
		degreeLabel += "° C";
		break;
	case "imperial":
		degreeLabel += "° F";
		break;
	case "default":
		degreeLabel = " K";
		break;
	}
}

currentweather

temperature.innerHTML = " " + this.temperature.replace(".", this.config.decimalSymbol) + degreeLabel;

weatherforecast

maxTempCell.innerHTML = forecast.maxTemp.replace(".", this.config.decimalSymbol) + degreeLabel;
minTempCell.innerHTML = forecast.minTemp.replace(".", this.config.decimalSymbol) + degreeLabel;

If these changes are okay with you, I will open two PR for this issue.

@MichMich
Copy link
Collaborator

MichMich commented Jan 3, 2019

A PR is more than welcome! Thanks!

@MichMich
Copy link
Collaborator

MichMich commented Jan 3, 2019

Merged.

@MichMich MichMich closed this as completed Jan 3, 2019
@fewieden
Copy link
Contributor

fewieden commented Jan 3, 2019

I will also change it for the new weather module.

Done will be in the next pr

@fewieden
Copy link
Contributor

fewieden commented Jan 5, 2019

Handled in #1510 for the new weather module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants