SeaTurtles - Adriana G. #70
Conversation
src/index.js
Outdated
| // const axios = require('axios') | ||
| // require('dotenv').config() | ||
|
|
||
| // const { default: axios } = require("axios"); | ||
|
|
||
| // console.log(document.getElementById('curr-loc')); | ||
|
|
||
| // console.log(process.env.LOCATIONIQ_KEY) | ||
|
|
||
| // let currTemp = parseInt(document.getElementById('temp-now').textContent); |
There was a problem hiding this comment.
We want to clean up unused code & comments before opening our PRs to make sure they don't make it into the main/deployment branch of our projects.
| let tempText = document.getElementById('temp-now'); | ||
| let highTempText = document.getElementById('hi-temp'); | ||
| let lowTempText = document.getElementById('low-temp'); |
There was a problem hiding this comment.
I like the idea of pulling element lookups to the top of the page so they're stored in a variable at the time we need them!
It looks like there's a mix styles across the file with these 3 elements being looked up & stored up here and other elements looked up directly in the functions. For consistency, we would want to pick one style to use across the project, with the caveat that if there is an element that is only conditionally aded to the DOM based on user events, that specific item may need to be looked up later inside a function when it is guaranteed to exist.
| if (STATE.currTemp < 50) { | ||
| chosenTempText.id = 'cold'; | ||
| chosenTherm.src = './assets/thermometer-icons/cold.png'; | ||
| } else if (STATE.currTemp < 60) { | ||
| chosenTempText.id = 'cool'; | ||
| chosenTherm.src = './assets/thermometer-icons/cool.png'; | ||
| } else if (STATE.currTemp < 70) { | ||
| chosenTempText.id = 'warm'; | ||
| chosenTherm.src = './assets/thermometer-icons/warm.png'; | ||
| } else if (STATE.currTemp < 80) { | ||
| chosenTempText.id = 'hot'; | ||
| chosenTherm.src = './assets/thermometer-icons/hot.png'; | ||
| } else { | ||
| chosenTempText.id = 'scorching'; | ||
| chosenTherm.src = './assets/thermometer-icons/scorching.png'; | ||
| } |
There was a problem hiding this comment.
When we have large if-statements like this where each if-block performs the same operations with slightly different values, it can be worth making a helper function that handles the updates to reduce places we're repeating code and could accumulate small typos. With the helper function, each of the ifs here would call that function passing the desired parameters rather than setting attributes themselves.
const updateTempColor = () => {
...
if (STATE.currTemp < 50) {
updateChosenTempTextAndImage('cold', './assets/thermometer-icons/cold.png');
} else if ...
}
...
const updateChosenTempTextAndImage = (category, imagePath) => {
chosenTempText.id = category;
chosenTherm.src = imagePath;
}| <source src="assets/sunny.mp4" type="video/mp4"> | ||
| </video> | ||
| <container class="page" id="page-1"> | ||
| <div id="spacer-1"></div> |
There was a problem hiding this comment.
I would strongly recommend using CSS to set padding & margins to create space between elements over adding empty block elements like divs
| <div id="content-1"> | ||
| <h1 id="title">What's the Weather?</h1> | ||
| <h2 id="subtitle">Dealer's choice!</h2> | ||
| <p><b>Ever wish you could control the weather?</b> Now you can! With | ||
| <b>What's the Weather</b> now anyone can decide what kind of day | ||
| it'll be! Simply play with the buttons and make | ||
| it <b><em>your day!</em></b></p> | ||
| </div> | ||
| <div class="scroll"> | ||
| <h3>Check the weather</h3> | ||
| <img src="./assets/down-chevron.png" alt="down chevron arrow"></img> | ||
| </div> |
There was a problem hiding this comment.
I would recommend replacing the divs here with elements that have more semantic meaning like section. This applies to most of the div uses I see across the HTML. We should reserve div for when no other semantic element makes sense or we need a container for styling that does not otherwise have meaning to the layout.
| <video playsinline autoplay muted loop poster="assets/down-chevron.png" id="bgvid-3"> | ||
| <source id="page3-conditions" src="assets/Lightning.mp4" type="video/mp4"> | ||
| </video> | ||
| <container class="page" id="page-3"> |
|
|
||
| return [latitude, longitude]; | ||
| }) | ||
| .then((response) => { |
There was a problem hiding this comment.
Nice use of .then chaining to fire the calls off in sequence! This function is a bit long, if we wanted to divide it up for clarity, we could move the /weather endpoint call to its own function and call the new function from a .then block here.
| displayCity.textContent = input; | ||
| }; | ||
|
|
||
| const displayChosenState = () => { |
There was a problem hiding this comment.
Really nice use of helper functions to divide up the actions we want to take into smaller portions that are easier to read and test.
| animation: neon-scorching .08s ease-in-out infinite alternate; | ||
| } | ||
|
|
||
| @keyframes neon-scorching { |
There was a problem hiding this comment.
Love the neon effects on the temperature!
| </span> | ||
| </div> | ||
| <div class="real-temp"> | ||
| <img id="thermometer-image" src="./assets/thermometer-icons/cold.png" alt="thermometer" /> |
There was a problem hiding this comment.
I like how the thermometers turned out! Very cute ^_^
…der into root folder
Hello!
Please note that I replaced the landscape requirement with a thermometer icon that changes depending on the temperature. I also replaced the drop-down menu for weather conditions to icons, and instead used the drop-down menu to select the state (I did this purely for aesthetics as I liked how the weather icons looked at the bottom of the screen.)
Please also note that there is SO much refactoring that needs to be done. I didn't have time to delete comments or fix the many bugs that exist, but this is what I have so far.
Improvements I would make if I had more time:
Feature I wanted to add: