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

Sea Turtles-Jenny C #79

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

Conversation

jennycodingnow
Copy link

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 Jenny! Your code was functional and I was able to do all of the required tasks in your weather app! I left some comments and shared some resources. Please let me know if you have any questions.

<h1>Weather Report</h1>
</header>
<main>
<div class="grid-display">
Copy link

Choose a reason for hiding this comment

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

Most of your code in HTML is semantic. I think in this particular place you could've used something else besides a div. Here is a document you can use to walk through when to use particular tags:
html_flowchart
html_flowchart

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much Trenisha.

Comment on lines +52 to +53
<footer>
</footer>
Copy link

Choose a reason for hiding this comment

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

you don't need the footer tag if you do not have a footer

Comment on lines +52 to +53
<footer>
</footer>
Copy link

Choose a reason for hiding this comment

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

you don't need the footer tag if you do not have a footer

@@ -0,0 +1,139 @@
'use strict';

const state = {
Copy link

Choose a reason for hiding this comment

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

👍🏽

@@ -0,0 +1,139 @@
'use strict';

const state = {
Copy link

Choose a reason for hiding this comment

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

👍🏽

Comment on lines +36 to +46
const increaseTemp = () => {
state.temperature += 1;
updateTempLabel();
console.log('increase temp');
};

const decreaseTemp = () => {
state.temperature -= 1;
updateTempLabel();
console.log('decrease temp');
};
Copy link

Choose a reason for hiding this comment

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

functions look good but remove your console logs


document.addEventListener('DOMContentLoaded', registerEventHandlers);

const getLatAndLong = () => {
Copy link

Choose a reason for hiding this comment

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

small note. I would move this function and the function below this before you register the event handlers. Just to make the code more readable.

Copy link
Author

Choose a reason for hiding this comment

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

ok, got it. I thought line 103 was only for all the events so I put it before the API calls. I see now that I put it at the end of index. Thank you


document.addEventListener('DOMContentLoaded', registerEventHandlers);

const getLatAndLong = () => {
Copy link

Choose a reason for hiding this comment

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

small note. I would move this function and the function below this before you register the event handlers. Just to make the code more readable.

styles/index.css Outdated
Comment on lines 30 to 31


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 this white space and any other place that has extra white space to make your code more readable

styles/index.css Outdated
Comment on lines 30 to 31


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 this white space and any other place that has extra white space to make your code more readable

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