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

Panthers--Maggie T #92

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Panthers--Maggie T #92

wants to merge 7 commits into from

Conversation

mjtice0
Copy link

@mjtice0 mjtice0 commented Dec 14, 2022

No description provided.

Comment on lines +15 to +40
<h1>Weather for <span id="city"></span></h1>
<div>
<input id="cityInput" type="text" size="30" placeholder="Enter city" />
<button id="citySearch">Search</button>
<button id="cityReset">Reset</button>
</div>

<h4>Temperature is <span id="temperature"></span>º F</h4>
<div>
<button id="decreaseTemp">-</button>
<button id="increaseTemp">+</button>
</div>
</div>

<div class = "landscapeSky">
<h4>Landscape is <span id="landscape"></span></h4>

<h4>Sky is <span id="sky"></span></h4>
<div>
<select id="skyInput">
<option value="sunny">Sunny</option>
<option value="cloudy">Cloudy</option>
<option value="rainy">Rainy</option>
<option value="snowy">Snowy</option>
</select>

Choose a reason for hiding this comment

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

Markup looks great!

Comment on lines +54 to +75
#skyInput {
margin: 0;
width: calc(100% - 200px);
font-size: 105%;
border: 1px solid purple;
background-color: antiquewhite;
color: black;
font-family: inherit;
padding: 1.5em 1em;
float: left;

}

#decreaseTemp{
font-size: 36px;
font-weight: 600;
}
#increaseTemp {
font-size: 28px;
font-weight: 600;
}

Choose a reason for hiding this comment

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

Amazing work with styling 👍🏾

Comment on lines +2 to +11
body {
display: flex;
justify-content: center;
align-items: center;
height: 100vh;
margin: 30px;
background-color: rgb(136, 55, 211);
font-size: 120%;
flex-direction: column;
}

Choose a reason for hiding this comment

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

I would suggest adding media queries to account for all types of screen sizes

Comment on lines +2 to +6
const state = {
sky: 'sunny',
temperature: 0,
city: '',
};

Choose a reason for hiding this comment

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

Instead of having a hard coded default sky state, how might you refactor this so the logic is more data driven? The sky should really be displayed based on the call to the api

temperature: 0,
city: '',
};
const temperatureChangeStep = 1;

Choose a reason for hiding this comment

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

Prefer += temp or -= temp here 😁

setTemperature(state.temperature - temperatureChangeStep);
};

const kelvinToFarenheit = (k) => 1.8 * (k - 273) + 32;

Choose a reason for hiding this comment

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

👍🏾


//use the city to get lat/long
const fetchCityLatLong = (city) => {
console.log('fetchCityLatLong()', city);

Choose a reason for hiding this comment

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

Remember to remove debugging log statements


// use lat/lon coordinates to get temperature
const fetchLatLongTemperature = (latLong) => {
const { lat, lon } = latLong;

Choose a reason for hiding this comment

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

Nice!

Comment on lines +30 to +33
elCityReset.addEventListener('click', (event) => {
setCity('');
elCityInput.focus();
});
Copy link

@ameerrah9 ameerrah9 Jan 11, 2023

Choose a reason for hiding this comment

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

There's an issue with wave 06 (resetting the city name), the requirements specify that you must include a button that resets the city name & when a user clicks on this button, the city name will be set to a default name along with the temperature for that city. Currently your code is not displaying the city name when you reset the city.

Choose a reason for hiding this comment

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

When you reset the city it should be updated to a selected city instead of an empty string like so:

cityNameInput.value = 'Seattle';

Comment on lines +166 to +174
//initialization - render all state first
renderCity();
renderTemperature();
renderSky();

//run initial search and show a temperature
setCity('Denver');
searchCityWeather('Denver');

Choose a reason for hiding this comment

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

Since your script file is included at the end of the HTML file, it's probably fine to invoke this directly. But prefer to register it to be called in response to the DOMContentLoaded event. In the similar live code we did, we did call our onLoaded function directly, since codesandbox got in the way of our code being able to see the DOMContentLoaded event.

Copy link

@ameerrah9 ameerrah9 left a comment

Choose a reason for hiding this comment

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

Great work on this project Maggie! The code you wrote for wave 6 is really close, I've put in a suggested solution. The code in this project is really clean and easy to read. This project is yellow 🟡

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