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

Cedar - Kristin L #57

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

Cedar - Kristin L #57

wants to merge 6 commits into from

Conversation

Kbhlee2121
Copy link

No description provided.

Copy link

@beccaelenzil beccaelenzil 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 vanilla javascript project. Your code is clear and you've clearly met the learning goals around dynamic styling and event handling. Nice work using both flexbox and grid to create a clean layout!


//reset City
const resetCity = (event) => {
event.target.value = 'Seattle';

Choose a reason for hiding this comment

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

Rather than setting the event.target.value to 'Seattle', we want to selected the input box itself, and set the value of that element to "Seattle". This will put 'Seattle' into the input box.

Comment on lines +78 to +91
skyDropdown.addEventListener('change', (event) => {
if (event.target.value === 'sunny') {
selectedSky.textContent = ' ☁️ ☁️ ☀️ ☁️ ☁️ ☀️ ☀️ ☀️ ☁️ ☀️☀️ ☀️ ☁️ ☀️';
gardenBackground.style.backgroundColor = '#D6FFFF';
} else if (event.target.value === 'cloudy') {
selectedSky.textContent = '☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️🌤 ☁️ ☁️☁️';
gardenBackground.style.backgroundColor = '#C9C9C9';
} else if (event.target.value === 'rainy') {
selectedSky.textContent = '🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧🌧🌈⛈🌧';
gardenBackground.style.backgroundColor = '#9FCFE0';
} else if (event.target.value === 'snowy') {
selectedSky.textContent = '🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨🌨❄️❄️🌨🌨';
gardenBackground.style.backgroundColor = '#A1B6D6';
}

Choose a reason for hiding this comment

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

To enhance readability, you might consider refactoring and moving the sky/color change functionality into a named function that's passed here as a callback function (rather than an anonymous function)

});
};

const defaultSetup = () => {

Choose a reason for hiding this comment

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

Nice work encapsulating this default set-up here!

const displayCity = document.getElementById('cityNameDisplay');
const resetButton = document.getElementById('resetCity');

const state = {

Choose a reason for hiding this comment

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

Nice use of the state object!

<main>
<section class ="content-wrapper">
<div class="items-wrapper">
<div class="item-wrapper" id="temperatureContainer">

Choose a reason for hiding this comment

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

You might consider making the temperatureContainer, cityNameContainer, skyContainer, and weatherGardenContainer section elements

gardenBackground.style.backgroundColor = '#9FCFE0';
} else if (event.target.value === 'snowy') {
selectedSky.textContent = '🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨🌨❄️❄️🌨🌨';
gardenBackground.style.backgroundColor = '#A1B6D6';

Choose a reason for hiding this comment

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

Directly applying styles works and is a valid approach. You may also consider applying a class name to keep the styles in the stylesheet.

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