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

C17 Otters Ying Goings #83

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

C17 Otters Ying Goings #83

wants to merge 15 commits into from

Conversation

yinggoings
Copy link

No description provided.

@yinggoings yinggoings changed the title basic stuffz 4 basic me C17 Otters Ying Goings Jun 16, 2022
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.

Wonderful work, Ying! 🎉

I'm so impressed with your JS and CSS savviness! You met all the project requirements and then some. I love so many things about this implementation, namely how the sky and the temperature colors create a unique gradient, all the error handling details, and the week's entire forecast! This project is a Green. 🟢

I'm so excited to see what you'll make out there! Keep it up! ⭐

}

document.addEventListener('DOMContentLoaded', registerEventHandlers);
document.addEventListener('DOMContentLoaded', displayStates);

Choose a reason for hiding this comment

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

Nit: Remember to put a newline at the end of files!

<section class="left-col">
<h1 id="city-name"></h1>
<div class="search-bar">
<input id="text-input" type="text">

Choose a reason for hiding this comment

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

To make this label even more screen-reader friendly, I recommend making the id name even more specific, something like city-name-text-input!

<h1 id="city-name"></h1>
<div class="search-bar">
<input id="text-input" type="text">
<button class="button" id="fetchweather"><img class="icon" src="./assets/getliveweather.png" alt="Fetch current weather conditions and weather forecast"></button>

Choose a reason for hiding this comment

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

Class name button here isn't all too informative since we could select all buttons using the element marker button in CSS,

</div>
<div id="error-box"></div>
<div class="skies-section">
<label for="skies">Change sky:</label>

Choose a reason for hiding this comment

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

Love the use of the for attribute for this label! 💯

Comment on lines +5 to +14
const state = {
title: initialTitle,
temp: 60,
sky: null,
unit: 'F',
land: null,
cur_conditions: null,
timezone: null,
toc: false
};

Choose a reason for hiding this comment

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

Great job setting the global state of this app at the top of the file!

Comment on lines +332 to +341
const displayStates = () => {
tempDisplay.textContent = Math.round(state.temp);
updateColor();
cityDisplay.textContent = state.title;
updateLandState();
updateSkyState();
cityCurConditions.textContent = state.cur_conditions;
displayDateTime();
updateForecast();
}

Choose a reason for hiding this comment

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

✨ Wonderful! ✨

}
}

const fetchAll = async () => {

Choose a reason for hiding this comment

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

Great implementation of a async/await function!

return localDateTime;
};

const displayDateTime = () => {

Choose a reason for hiding this comment

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

Going above and beyond! 🚀

@@ -0,0 +1,78 @@
body {

Choose a reason for hiding this comment

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

Love the craiglist-chic minimalism and the gradients! 🎉

await fetchAll();
};
const error = (err) => {
alert('Your browser does not permit location sharing.');

Choose a reason for hiding this comment

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

Great use of JS alerts!

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