Accelerate - Karolina Benitez #4
Conversation
There was a problem hiding this comment.
Nice job!
Your HTML is really clean, and you accomplished a lot with very few CSS rules, making a really beautiful site. The deployed version is running well too, so good job there.
In CSS, I'd recommend trying to apply flex/grid display a bit more aggressively to reduce the need to use margins for layout. Also, think about how to handle the UI at various sizes. The min-width rule can be really helpful for preventing our layout from getting completely squished. Beyond that, media queries can provide a rich set of tools for providing custom styles for displays of various sizes.
In JS, I'd love to see you take the data-centric approach you had for the temperature a bit further, so that the logic for determining which temperature configuration gets applied is itself data-driven. The same approaches could be applied to the sky selection so that we can dynamically add new skies and temperatures and everything would "just work".
In pairing JS with CSS, consider dynamically applying classes to elements by using the className property so that we can have rich styles applied all at once, without need to know in our JS code which style properties need to be affected.
Overall, well done!
| @@ -1,5 +1,5 @@ | |||
| # Weather Report | |||
|
|
|||
| # https://karolina-benitez.github.io/weather-report/ | |||
There was a problem hiding this comment.
👍
Looks like your deployed copy is working great!
| @@ -0,0 +1,33 @@ | |||
| #mainBackground { | |||
There was a problem hiding this comment.
How does your page look so amazing with such a concise amount of styling?!?! 😮
| #mainBackground { | ||
| background-image: url("https://images.unsplash.com/photo-1473496169904-658ba7c44d8a?ixlib=rb-1.2.1&ixid=MnwxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8&auto=format&fit=crop&w=1650&q=80"); | ||
| /* Full height */ | ||
| height: 100%; |
There was a problem hiding this comment.
To make sure the image fills the window, try 100vh rather than 100%. In that case it's also a good idea to set margin: 0; or you're likely to get pesky scroll bars!
| background-size: cover; | ||
| } | ||
|
|
||
| .mainContent { |
There was a problem hiding this comment.
Rather than using a fixed margin to approximately center the control area, try making body into a flex container as noted above. Then remove the margin stuff here, and you should end up with a centered control area!
| background-position: center; | ||
| background-repeat: no-repeat; | ||
| background-size: cover; | ||
| } |
There was a problem hiding this comment.
To get the control area to center in the page area, try setting display: flex; here, along with justify-content: center; and align-items: center;
| mainContent.style.backgroundColor = "#c9c9c3" | ||
| mainBackground.style.backgroundImage = `url(${sky_info.sunny})` | ||
| skyContent.textContent = `☁️ ☁️ ☁️ ☀️ ☁️ ☁️`; |
There was a problem hiding this comment.
I'd love to see a more data-centric approach as you did for the temperature. If these values were stored in a data structure, we could both use iteration to find the object that has the values we want to apply to the UI, but we could also build the options in the sky select from the same data!
| const cityName = document.querySelector("#cityName"); | ||
| cityName.textContent = state.cityName; |
There was a problem hiding this comment.
Consider making these lines their own helper function that could be called from both here and from clearCityName.
| }; | ||
|
|
||
| const changeSky = (event) => { | ||
| const skyContent = document.querySelector("#skyContent"); |
There was a problem hiding this comment.
If looking an element up by id, prefer getElementById. querySelector with an an #id would be more appropriate if we need to get a child of that element, like document.querySelector("#someId p").
| renameCity.addEventListener("input", changeCityName); | ||
| const resetCityName = document.querySelector("#resetButton"); | ||
| resetCityName.addEventListener("click", clearCityName); | ||
|
|
There was a problem hiding this comment.
Similar to the suggestion to call the functions to update the temperature UI from here, also call the sky and city updates, so that everything reflects the default data values.
| } | ||
|
|
||
| const registerEventHandlers = (event) => { | ||
| const tempDownButton = document.querySelector("#tempDown"); |
There was a problem hiding this comment.
We could also load all the UI elements we need to work with here, and store them in our state so that we don't have to keep looking them up each time a change occurs.
No description provided.