sea turtles ada c17 #82
Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice job!
Storing app state in variables is a useful code strategy (get your sky in there too!). Structuring our code so that there are separate functions for updating those variables, and then publishing the values helps us manage user interactions and data processing. Think about how to restructure the axios calls to be a bit more generally reusable (how can we avoid making the second call directly from the first?). And keep an eye out for areas where we can use data structures to reduce code duplication.
|
|
||
|
|
||
| </main> | ||
| <script src="/src/index.js"></script> |
There was a problem hiding this comment.
Depending on your deployment target, absolute paths (starting with /) might not work properly. For project-style github pages sites, we need to use only relative paths (not starting with /) since they are deployed to a subdirectory, which throughs off what absolute paths map to.
|
|
||
|
|
||
|
|
||
| <section id="choose_sky" class="box"> |
There was a problem hiding this comment.
A section without a heading generates a validation warning. If we're really just grouping some elements for styling and not grouping them under a heading of some kind, prefer a plain div.
| color:darksalmon; | ||
| padding: 2rem; | ||
| border-radius: 50px; | ||
| background-image: url(/styles/glitter.png); |
There was a problem hiding this comment.
In styles, still try to use relative paths to resources. They are interpreted relative to the css file location, not the HTML importing it.
| @@ -0,0 +1,164 @@ | |||
| body{ | |||
There was a problem hiding this comment.
Try using the VSCode Format Document option to keep the CSS file tidy. The indentation is a bit inconsistent.
| @@ -0,0 +1,199 @@ | |||
| "use strict"; | |||
There was a problem hiding this comment.
Remember to make git commits as you work. The README asked us to make at least 5 commits (1 per wave is a good start).
| console.log(response.data.current.weather[0]['main']); | ||
|
|
||
| const weather = response.data.hourly[0]['temp']; | ||
| state.temperature = Math.round(1.8*(weather-273.15)+32); |
There was a problem hiding this comment.
Consider moving this conversion code into a helper function.
| const findLatAndLong = () => { | ||
| console.log(state.city); | ||
| axios | ||
| .get('http://localhost:5000/location', { |
There was a problem hiding this comment.
Consider storing the unchanging part of the URL (the base URL) in a variable elsewhere, then use string interpolation to build the full path here. This would make it easier to change where all of the endpoints in the app are going by changing a single variable.
|
|
||
| let sky = ''; | ||
| let skyColor = ''; | ||
| if (inputSky === 'cloudy') { |
There was a problem hiding this comment.
Like for the temp check, each of these clauses is very repetitive. We look for a matching value to locate information about what values to set in the UI. Consider using object (key) lookups to make this more data-driven. If the values are stored elsewhere (maybe loaded from an api) then the code here would essentially reduce down to a key lookup in an object!
| const inputSky = document.getElementById('changeSky').value; | ||
| const skyContainer = document.getElementById('sky'); |
There was a problem hiding this comment.
Consider storing selected value in a state variable, and splitting the updating the state into a separate function from applying the state to the UI.
| const cityNameInput = document.getElementById('cityName'); | ||
| cityNameInput.value = 'New Orleans'; | ||
|
|
||
| updateCityName(); |
2 participants
No description provided.