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.

Sign up for GitHub

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

Emily - HW 04 #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Emily - HW 04 #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@


# Build a Weather App

Expand Down
11 changes: 11 additions & 0 deletions css/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
* {
font-family: Helvetica, Arial, sans-serif;
}

body {
background-color: white
}

current-temp {

}
20 changes: 20 additions & 0 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,30 @@

<body>

<header>
Copy link
Contributor

Choose a reason for hiding this comment

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

@kcycle nice updates to the UI

<div class="title">
<center><h1>Weather Finder</h1></center>
</div>
</header>

<div class="container">
<center><div>
<input type="text" id="city-type" placeholder="Enter a city name...">
<input type="submit" value="Submit" id="submit-btn">
</div></center>
<center><div id='current-city'></div></center>
<center><div id='current-temp'></div></center>
<center><div id = 'min-temp'></div></center>
<center><div id = 'max-temp'></div></center>
<center><div id = 'description'></div></center>

</div>

<script
src="https://code.jquery.com/jquery-3.4.1.min.js"
integrity="sha256-CSXorXvZcTkaix6Yvo6HppcZGetbYMGWSFlBw8HfCJo="
crossorigin="anonymous"></script>
<script src="https://unpkg.com/axios/dist/axios.min.js"></script>
<script src="js/app.js"></script>
</body>
</html>
89 changes: 88 additions & 1 deletion js/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,89 @@
$(function () {
$(document).ready(function () {
$('#submit-btn').click(() => {

refreshSearch()

const city = $('#city-type').val()

getWeatherByCity(city)

})


function getWeatherByCity (city) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kcycle nice work using the standard promise syntax here.

axios.get(`http://api.openweathermap.org/data/2.5/weather?q=${city}&APPID=1c739a9713a0919a3024a089c3b027df`)
Copy link
Contributor

Choose a reason for hiding this comment

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

@kcycle it's recommended that you use https instead of http whenever available. The use of http here may subject you to a CORS error.

.then((response) => {
console.log(response.data)
displayCityName(response.data.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

@kcycle nice! I like the use of separate functions to display each element. Putting all the display logic into one function would have also adhered to the SRP rule as well.

displayCurrentTemperature(convertToFahrenheit(response.data.main.temp))
displayWeatherDescription(response.data.weather[0].description)
displayMinTemp(convertToFahrenheit(response.data.main.temp_min))
displayMaxTemp(convertToFahrenheit(response.data.main.temp_max))

}).catch((error) => {
console.log(error)
})

}




function displayCityName (cityName) {
$('#current-city').append(
`<h4>Current City:</h4> <p>${cityName}</p>`)
}

function displayCurrentTemperature (temperature) {
$('#current-temp').append(
`<h4>Current Temperature:</h4> <p>${temperature} F°</p>`)
setTempFontColor(temperature)
}

function displayWeatherDescription (description) {
$('#description').append(
`<h4>Description:</h4> <p>${description}</p>`)
}

function displayMinTemp (temperature) {
$('#min-temp').append(
`<h4>Today's Low:</h4> <p>${temperature} F°</p>`)
}

function displayMaxTemp (temperature) {
$('#max-temp').append(
`<h4>Today's High:</h4> <p>${temperature} F°</p>`)
}

function convertToFahrenheit (kelvinUnits) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kcycle nice job creating a separate function to convert from Celcius to Fahrenheit. However, OpenWeatherMap's API does this work for you by passing an additional request parameter with the API call: units: 'imperial' - will return the temperature in Fahrenheit

See the docs here: https://openweathermap.org/current#data

And here's how I use it in the solution: https://github.com/jsd20191008/weather-api-app-solution/blob/master/js/main-promises.js#L56

return parseInt((kelvinUnits-273.15)*(9/5)+32)
}

function setTempFontColor (temperature) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kcycle great job breaking out this logic into a separate function.

the only suggestion here would be to use if..elseif..esle logic instead of two separate if statements. Take a look at the approach used in the solution: https://github.com/jsd20191008/weather-api-app-solution/blob/master/js/main-promises.js#L82

if (temperature > 85) {
$('#current-temp').css({
color: "red"
})
}
if (temperature < 40) {
$('#current-temp').css({
color: "blue"
})
}
}

function refreshSearch () {
$('#current-city').html('')
$('#current-temp').html('')
$('#description').html('')
$('#min-temp').html('')
$('#max-temp').html('')
}


Copy link
Contributor

Choose a reason for hiding this comment

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

@kcycle be careful regarding unnecessary blank lines...will not result in an error, but it makes the code harder to scan.






})