-
Notifications
You must be signed in to change notification settings - Fork 6
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
Implementation WeatherForecast #468
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @fdelcampo ! I really appreciate you improved and refactored several things. I left comments regarding code quality and a few suggestion, please take a look to them 🙏️
@@ -0,0 +1,10 @@ | |||
.svg{ | |||
height: 35px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In base to the latest work we are doing with screen sizes... Do you think this should be set with em
units?
} | ||
|
||
.icon { | ||
width: 50px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In base to the latest work we are doing with screen sizes... Do you think this should be set with em units?
@@ -0,0 +1,10 @@ | |||
.svg{ | |||
height: 35px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In base to the latest work we are doing with screen sizes... Do you think this should be set with em units?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented lines should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this moment not used with a refactor. This file removed
const currentTemperature = this.props.weather?.ambient_temp?.value; | ||
const currentHumidity = this.props.weather?.humidity?.value; | ||
const currentPressure = Math.round(this.props.weather?.pressure?.value * 100) / 100; | ||
const currentPressure = this.props.weather?.pressure?.value; // Math.round(this.props.weather?.pressure?.value * 100) / 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment should be removed.
/* bar: { | ||
continuousBandSize: 5, | ||
}, */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines should be removed.
}; | ||
} | ||
|
||
makeAxisTitle = (title, units) => { | ||
return `${title.toUpperCase()} ${units ? `[${units}]` : ''}`; | ||
return `${title} ${units ? `[${units}]` : ''}`; | ||
// return `${title.toUpperCase()} ${units ? `[${units}]` : ''}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be removed.
// { | ||
// calculate: 'datum.colors[datum.name]', | ||
// as: 'color' | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines should be removed.
// domain: [-1, 10], | ||
// rangeMin: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines should be removed.
// domain: [-1, 10], | ||
// rangeMin: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines should be removed.
5f25a9b
to
dbc31d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to be merged 👍
…ue for angle of arrow, and second position in area
…Pending deprecate TimeSeriesConfig
… maxHeight for the Plot
… to div of the plot
… and delta minus.
… the name of subscription
…e plot independently to the view
…ize when show in screen 4K
…e plot line. Change the plot of the MTDome and Dome, for add axis-y left and right
…the height plot line < 240, when it's without the rule of hover
7018566
to
461348d
Compare
No description provided.