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

Weather Report - HW - Sea Turtles #76

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

Conversation

hopeolaide
Copy link

Had to close previous pull request. Wrong branch and some issues tracking some missing code. Still trouble-shooting a bit but everything seems to be here for the most part.

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 Hope! Everything is working 💃🏽 ! I left some comments on resources you can use to clean up what we discussed in our 1:1! Let me know if you have questions!

<body>

<div>Weather Report</div>
<main>
<section>
Copy link

Choose a reason for hiding this comment

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

good job using sections and other semantic HTML Here is a resource to show you when to use particular tags
html_flowchart

'https://images.unsplash.com/photo-1584267385494-9fdd9a71ad75?ixlib=rb-1.2.1&ixid=MnwxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8&auto=format&fit=crop&w=2670&q=80',
};

const domElements = {
Copy link

Choose a reason for hiding this comment

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

adding your domElements here is a good way to organize your file

domElements.currentTemp.textContent = state.temperature;
},
tempBackgroundChange: (backgroundColor, landscape) => {
console.log(backgroundColor);
Copy link

Choose a reason for hiding this comment

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

remove your console log here

return Math.floor(1.8 * (kelvin - 273) + 32)

}
const setBackgroundColorLandscape = (temperature) => {
Copy link

Choose a reason for hiding this comment

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

👍🏽

}
};

const setSkyImage = (skyCondition) => {
Copy link

Choose a reason for hiding this comment

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

👍🏽

};

// const axios = require('axios');
const getLatitudeLongitude = async () => {
Copy link

Choose a reason for hiding this comment

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

I would move your api functions before registering your event handlers just to put them all together with the other functions

@@ -0,0 +1,65 @@
body {
Copy link

Choose a reason for hiding this comment

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

I know we talked about using the grid to overlap elements. Here is a resource to do that https://www.joomlashack.com/blog/tutorials/layering-items-within-css-grid/

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