Skip to content

initial commit#214

Open
areebakwas wants to merge 1 commit intoprojectshft:masterfrom
areebakwas:master
Open

initial commit#214
areebakwas wants to merge 1 commit intoprojectshft:masterfrom
areebakwas:master

Conversation

@areebakwas
Copy link
Copy Markdown

No description provided.

Comment thread index.html
</head>
<body>
<div class="container">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove empty spaces

Comment thread index.html
Comment on lines +21 to +23



Copy link
Copy Markdown

Choose a reason for hiding this comment

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

no need for empty spaces, affects the code flow

Comment thread main.js
// create eventlistener and variable

document.getElementById('button-search').addEventListener('click', function() {
let townName = document.getElementById('current-town').value;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why let ? this is should be a const, check ES6 best practices

Comment thread main.js
// create eventlistener and variable

document.getElementById('button-search').addEventListener('click', function() {
let townName = document.getElementById('current-town').value;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

here is where you are missing the empty input validation. Very important to consider, or your app will crash

Comment thread main.js

document.getElementById('button-search').addEventListener('click', function() {
let townName = document.getElementById('current-town').value;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

empty spaces

Comment thread main.js
Comment on lines +14 to +22
const apiKey = '63168e96197ab571649bdefbef398926';
const currentWeatherApiUrl = `https://api.openweathermap.org/data/2.5/weather?q=${townName}&units=imperial&appid=${apiKey}`;
//create fetch to grab data, add error check as well
fetch(currentWeatherApiUrl)
.then(response => response.json())
.then(data => {
showPresentWeather(data);
predictClimate(townName, apiKey)
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

your code alignment is off

Comment thread main.js
//create fetch to grab data, add error check as well
fetch(currentWeatherApiUrl)
.then(response => response.json())
.then(data => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

be more descriptive, data tells not much,

Suggested change
.then(data => {
.then(weatherData => {

Comment thread main.js
}
//create fxn fivedayweather

function fiveDayWeather(data) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

use arrow functions next time

Comment thread main.js

//find last town searched and save
document.getElementById('button-search').addEventListener('click', function() {
let townName = document.getElementById('current-town').value;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why let?

Comment thread main.js

});
window.onload = function() {
let previousTown = localStorage.getItem('previousTown');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

again, should be const

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.

2 participants