Skip to content

Conversation

@ymmahmoud
Copy link

Created a weather column to fill some of the whitespace between the rider names/900s and the call volume blocks. This weather column pulls data from the openweathermap API and automatically updates icon colors based on light mode vs. dark mode. It currently shows 4 days of weather because any more would cause notes and chores to be pushed down.

@ymmahmoud ymmahmoud requested a review from lramos15 March 3, 2020 05:41
@lramos15
Copy link
Contributor

lramos15 commented Mar 3, 2020

Can you fix the JavaScript file to use ES6 notation such as let instead of var. Also you can remove the dependency on jQuery as current JavaScript standards make it pretty much obsolete. I also request that you fix the code climate issue above. Code climate is our automatic structure checker which ensures that the code is maintainable. Lastly, you need to bump the version in package.json

Copy link
Contributor

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

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

See PR comment

Copy link
Contributor

@zapdos26 zapdos26 left a comment

Choose a reason for hiding this comment

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

See comment

@@ -0,0 +1,70 @@
// Set API key, longitude, and latitude
const api = "232f1a91a4893327ac3317c3786ac2be";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the hardcoded API key from the file and instead add it to the environment variable.

@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 5e5b2b0 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@ddbruce
Copy link
Member

ddbruce commented Apr 4, 2020

@ymmahmoud bump

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

Successfully merging this pull request may close these issues.

5 participants