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

Otters | Tori Shade #85

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

Otters | Tori Shade #85

wants to merge 2 commits into from

Conversation

ToriShade
Copy link

No description provided.

Copy link

@goeunpark goeunpark 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, Tori! 🎉

Your JS looks great and you have hit all the learning objectives! This project is a Green. 🟢

Keep it up!

city: "Seattle",
latitude: 47.6038321,
longitude: -122.3300624,
climate: temperature,

Choose a reason for hiding this comment

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

What would be temperature here?

@@ -0,0 +1,323 @@
/* Page Background */
html {

Choose a reason for hiding this comment

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

Some bits of code (like the following) are repeated across the file. We can DRY css by stating it once in a more encompassing element, like in html:

font-family: 'Inter';
font-style: normal;
color: #FFFFFF;

top: 50%;
left: 50%;
margin-right: -50%;
transform: translate(-50%, -50%);

Choose a reason for hiding this comment

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

Since #main-title and #sub-title also has a lot of overlapping information, we could pull that info out of the individual selectors and add them to a combined CSS block:

#main-title, #sub-title {
  position: absolute;
  left: 50%;
  margin-right: -50%;
  transform: translate(-50%, -50%);
  etc...
}

document.addEventListener('DOMContentLoaded', getCurrentForecast);
document.addEventListener('DOMContentLoaded', changeTemperatureAndLandscapeStyling);
document.addEventListener('DOMContentLoaded', setTheMood);
document.addEventListener("DOMContentLoaded", registerEventHandlers);

Choose a reason for hiding this comment

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

Nice use of the helper method registerEventHandlers! ✨

Comment on lines +55 to +59
<!-- <section>
<button id="addCrabButton">Add Crab</button>
</section>
<section id="crabContainer">
</section> -->

Choose a reason for hiding this comment

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

Remember to remove any superfluous code! 😉

Comment on lines +23 to +28
const cityNameInput = document.getElementById("city-name-input");
cityNameInput.addEventListener("keyup", () => {
currentLocation.city = cityNameInput.value;
displayCurrentLocation();
});
}

Choose a reason for hiding this comment

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

For ever cleaner code, consider moving this event listener down to registerEventHandlers and refactoring this function.

changeTemperatureAndLandscapeStyling();
})
.catch((error) => {
console.log(`Current Temperature for ${currentLocation.city} Not Found ${error.response.data}`);

Choose a reason for hiding this comment

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

Good error handling message!

Comment on lines +8 to +13
window.onload = () => {
displayCurrentLocation();
getCurrentForecast();
setTheMood();
changeTemperatureAndLandscapeStyling();
};

Choose a reason for hiding this comment

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

Great job researching and implementing a window.onload function!

<div id = "sky-content-box"></div>
<h1 id = "sky-content-box-header">Sky</h1>
<select id = "sky-drop-down">
<option value="What's the Vibe?">--What's the Vibe?--</option>

Choose a reason for hiding this comment

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

What is the vibe, indeed.

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