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

Diana S - C19 Kunzite #53

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

Diana S - C19 Kunzite #53

wants to merge 6 commits into from

Conversation

d3897
Copy link

@d3897 d3897 commented Jun 12, 2023

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Well done, Diana! Your JavaScript syntax looks very good. You remembered your curly braces and semicolons well!

Things to consider:

  • Turn any traditional functions into arrow functions. It's where ECMAScript (standards and practices for JavaScript language) is headed.

Comment on lines +14 to +17
const state = {
temperature: 68,
city: 'Montreal'
};

Choose a reason for hiding this comment

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

Let's put this at the top, so it's easy to find our important data immediately

};


function updateColorLandscape() {

Choose a reason for hiding this comment

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

Try to use arrow functions as much as possible. You've got them in many other places, so let's keep it consistent with arrow functions.

Comment on lines +25 to +41
if (state.temperature >= 80) {
tempValue.className = "red";
landscape.textContent = "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂";
} else if (state.temperature >= 70 && state.temperature <= 79) {
tempValue.className = "orange";
landscape.textContent = "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷";
} else if (state.temperature >= 60 && state.temperature <= 69) {
tempValue.className = "yellow";
landscape.textContent = "🌾🌾_🍃_🪨__🛤_🌾🌾🌾_🍃";
} else if (state.temperature >= 50 && state.temperature <= 59) {
tempValue.className = "green";
landscape.textContent = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲";
} else if (state.temperature <= 49) {
tempValue.className = "teal";
landscape.textContent = "🌲🌲⛄️🌲⛄️🍂🌲🍁🌲🌲⛄️🍂🌲";
}
headerCityName.textContent = cityNameInput.value;

Choose a reason for hiding this comment

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

Another way to format this method would be to have an object that has temp values as keys, and then you could have values be a list. The list could hold background color and background image, like:

const tempDisplayObject = {
    80: ['red',  "🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂"], 
    70: ['orange', "🌸🌿🌼__🌷🌻🌿_☘️🌱_🌻🌷"]
} 

When you have an object, you don't need to do if/else if/else if/ else which can introduce bugs and also adds complexity to the method. If you have an object, than you can iterate over the object and check the keys with state.temperature and then get the correct values to set the color and landscape emojis.

Comment on lines +45 to +57
increaseTempControl.addEventListener("click", increaseTemperature);
decreaseTempControl.addEventListener("click", decreaseTemperature);
cityNameInput.addEventListener("input", updateColorLandscape);
currentTempButton.addEventListener("click", function() {
findLatitudeLongitude()
});


cityNameReset.addEventListener("click", function() {
cityNameInput.value = "Montreal";
state.temperature = 68;
updateColorLandscape();
});

Choose a reason for hiding this comment

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

Consider collecting these event listeners into a callback function, like registerEventHandlers, which will then be accessible once the DOM has completely loaded. Something like:

const registerEventHandlers = () => {
    // your event listeners here
};
document.addEventListener('DOMContentLoaded', registerEventHandlers);

This will ensure the event handlers find the appropriate HTML tags they need.

Comment on lines +66 to +82
if (selectedSky === "sunny") {
gardenContent.textContent = "☁️ ☁️ ☁️ ☀️ ☁️ ☁️";
gardenContent.className = "sunny"
console.log("☁️ ☁️ ☁️ ☀️ ☁️ ☁️");
} else if (selectedSky === "cloudy") {
gardenContent.textContent = "☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️";
gardenContent.className = "cloudy"
console.log("☁️☁️ ☁️ ☁️☁️ ☁️ 🌤 ☁️ ☁️☁️");
} else if (selectedSky === "rainy") {
gardenContent.textContent = "🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧";
gardenContent.className = "rainy"
console.log("🌧🌈⛈🌧🌧💧⛈🌧🌦🌧💧🌧🌧");
} else if (selectedSky === "snowy") {
gardenContent.textContent = "🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨";
gardenContent.className = "snowy"
console.log("🌨❄️🌨🌨❄️❄️🌨❄️🌨❄️❄️🌨🌨");
}

Choose a reason for hiding this comment

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

Another candidate for an object of key-value pairs with the keys as the sky conditions and the value is the emojis




const updateTemperature = function(far) {

Choose a reason for hiding this comment

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

Change to an arrow function


//dropdown
const skySelect = document.getElementById("skySelect");
skySelect.addEventListener("change", function() {

Choose a reason for hiding this comment

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

Change to an arrow function

});


cityNameReset.addEventListener("click", function() {

Choose a reason for hiding this comment

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

Change to an arrow function

increaseTempControl.addEventListener("click", increaseTemperature);
decreaseTempControl.addEventListener("click", decreaseTemperature);
cityNameInput.addEventListener("input", updateColorLandscape);
currentTempButton.addEventListener("click", function() {

Choose a reason for hiding this comment

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

Change to an arrow function

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