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

Heather M - Sea Turtles C17 #16

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

Heather M - Sea Turtles C17 #16

wants to merge 7 commits into from

Conversation

cathos
Copy link

@cathos cathos commented Jun 13, 2022

No description provided.

Copy link

@tgoslee tgoslee left a comment

Choose a reason for hiding this comment

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

Great job Heather! I liked your weather app! It was clean and simple and I liked ASCII art instead of emojis. For wave 3, when I typed a city in the input it didn't change anywhere on your app. I also couldn't get the current city's weather. I would check on the type of event listener you are using and where you are intending the city to appear. The temperature worked fine and so did changing the sky/landscape. Take a look at my feedback and let me know if you have questions.

Comment on lines +23 to +31
<figure id="weather_display">
<text id="numeric_temperature">25</text>
<div id="units_div"><button id="temperature_units">°C</button></div>
</figure>
<figure id="change_temperature">
<button id="decrease"alt="Decrease Temperature">&nbsp;-&nbsp;</button>
<button id="reset"alt="Reset Temperature">⃔&nbsp;&nbsp;</button>
<button id="increase"alt="Increase Temperature">&nbsp;+&nbsp;</button>
</figure>
Copy link

Choose a reason for hiding this comment

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

I was curious why you decided on the figure tag. Normally you would see it with an img tag so you could embed an image with a caption. You could also use it for diagrams or listing code. An example here: https://www.geeksforgeeks.org/html5-figure-tag/

Here is an example of a flowchart you could look at for guidance on what tags to use for that focus on semantic html:
html_flowchart

index.html Outdated
Comment on lines 13 to 22
<header>
<a id="site_title" href="index.html">
<p>~ current temperature in ~</p>
</a>
<nav>
<div><button id="search_location" alt="Search or Enter Location">⌖</button></div>
<input type="search" id="location_text" value="Denver" placeholder="location"></input>
<div><button id="refresh_weather" alt="Refresh">⇢</button></div>
</nav>
</header>
Copy link

Choose a reason for hiding this comment

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

looks good

<option value="snowy">snowy</option>
</select>
</div>
<div id="sky_display" class="ascii_art">
Copy link

Choose a reason for hiding this comment

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

I like this different approach to the sky display and landscape display

</pre>
</div>
<div id="landscape" class="ascii_art">
<pre>
Copy link

Choose a reason for hiding this comment

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

good use of pre tag to keep your sky/landscape formatted

Comment on lines +115 to +117
// document.getElementById('#temperature_units').textContent;
// state.tempUnits = currentUnits;
console.log('current units: ' + currentUnits);
Copy link

Choose a reason for hiding this comment

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

you can remove your console log and commented code

});
};

const changeCity = () => {
Copy link

Choose a reason for hiding this comment

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

When I am typing in the input it should change the city name. From wave 3:

  • A text input element that allows the user to change the city name
    You should have an event listener for input like
 const cityNameInput = document.getElementById('cityNameInput');
  cityNameInput.addEventListener('input', updateCityName);

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