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

Sharks - Sana Pournaghshband #69

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added assets/1989 sky.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/49.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/50.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/60.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/70.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/75.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/80.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/fearless.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/lover.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/red.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/rep backup.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/speak now.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/ts1 sky.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added assets/ts3 backup.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
75 changes: 72 additions & 3 deletions index.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,78 @@
<meta charset="UTF-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<link href="styles/index.css" rel="stylesheet">
<!-- <script src="./src/index.js"></script> -->
<script src="./node_modules/axios/dist/axios.min.js"></script>

Choose a reason for hiding this comment

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

Looks like you have two script tags that brings loads axios. This same element is also on line 78.

Be sure to remove this script at the beginning of the page since "When the browser encounters a <script> tag, it stops loading the HTML document. Instead, the browser pauses to download the entire script, which might take a long time to load. The browser continues rendering the page only after the script has finished downloading." (from our Learn reading)

https://learn-2.galvanize.com/cohorts/3169/blocks/1449/content_files/adding-behavior/connecting-html-css-and-js.md

<title>Weather Report</title>
</head>
<body>

<body id="background">
<header>
<h1> Sana's Swiftie Weather Report </h1>
</header>

<section id="location">
<button id="reset" class="clicky">
Reset
</button>

<form id="search">
<input type="text" id="city" class="box" value="London">
<button id="go" class="clicky" type="submit">
Go
</button>
</form>


<select id="skySelect" class="box">
<option>Lover</option>
<option>rep</option>
<option>1989</option>
<option>Red</option>
<option>Speak Now</option>
<option>fearless</option>

</select>
</section>
Comment on lines +17 to +39

Choose a reason for hiding this comment

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

I think that these elements could be nested with the main element since this is content is a main feature of your weather report page.




<main>
<section id="temp" class="card">
<h2 id="cityDisplay">
London
</h2>

<div class="tempDisplay">
<button id="left" class="arrow"></button>
<div id="degrees">
80
</div>
<div id="unit">
°F
</div>
<button id="right" class="arrow"></button>
</div>

<button id="fOrC" >
°C | °F
</button>
</section>

<section id="clothes" class="card">
<p id="lyrics">
a perfect night to dress up like hipsters
</p>
<img id='outfits' alt=".." src="assets/70.jpeg"/>
</section>
</main>

<footer>
Designed by Sana Pournaghshband (2022)
</footer>

<script src="src/index.js"></script>
<script src="./node_modules/axios/dist/axios.min.js"></script>
</body>
</html>
</html>

197 changes: 197 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
'use strict';

const state = {
temperature: 79,
city: 'London',
isF: true,
};

const convertKtoC = (temp) => temp - 273.15;
const convertKtoF = (temp) => (temp - 273.15) * (9 / 5) + 32;
const convertCtoF = (temp) => temp * (9 / 5) + 32;
const convertFtoC = (temp) => (temp - 32) * (5 / 9);
Comment on lines +9 to +12

Choose a reason for hiding this comment

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

👍


const getLatAndLon = (city) => {
return axios
.get('http://127.0.0.1:5000/location', {

Choose a reason for hiding this comment

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

Consider using a constant variable like LOCATION_IQ_URL here instead of a string literal for the URL.

params: {
q: city,
},
})
.then((response) => {
console.log(response.data);
return {
lat: response.data[0].lat,
lon: response.data[0].lon,
};
})
.catch((error) => {
console.log('Error finding the latitude and longitude:', error.response);
});
};

const getWeatherKelvin = (lat, lon) => {
return axios
.get('http://127.0.0.1:5000/weather', {

Choose a reason for hiding this comment

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

You can use a constant variable like WEATHER_URL instead of string literal here.

"Constants free the programmer from having to remember what each literal should be. Often values that stay constant throughout the program have a business meaning. If there are several such values, the programmer can define them all in the beginning of the program and then work with the easier-to-remember constant names." From: https://www.diffen.com/difference/Constant_vs_Literal

params: {
lat: lat,
lon: lon,
},
})
.then((response) => {
console.log(response.data);

Choose a reason for hiding this comment

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

Remember to remove print statements that you used for debugging, it helps keep your code concise and readable.

return response.data.current.temp;
})
.catch((error) => {
console.log('Error finding the latitude and longitude:', error.response);
});
};

const handleCityEntered = (event) => {
console.log(`City entered ${event}`);
const inputCityName = event.target.value;
const displayCityName = document.getElementById('cityDisplay');
state.city = inputCityName;
displayCityName.textContent = state.city;
};

const increaseTemp = (event) => {
state.temperature += 1;
colorChange();
};

const decreaseTemp = (event) => {
state.temperature -= 1;
colorChange();
};

const reset = (event) => {

Choose a reason for hiding this comment

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

The name reset is a bit vague. It might leave people wondering 'what are we resetting?'

Consider renaming this method to something like resetCityAndWeather to be more descriptive

// state.temperature = 79; //change it after API

Choose a reason for hiding this comment

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

Remove commented out code so it doesn't accidentally get uncommented, if that happens then code would execute incorrectly and might create a bug in your site.

state.city = 'London';

Choose a reason for hiding this comment

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

Since London is your default city, you could use a constant like DEFAULT_CITY. I like to use constants because they can be reused if needed in the future and they can convey content about what the code is doing.

document.getElementById('city').value = state.city;
document.getElementById('cityDisplay').textContent = state.city;

go(event);
};

const go = (event) => {

Choose a reason for hiding this comment

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

This method is doing a lot since it gets lat/lon and weather for city, but the method name does not indicate what it's actually doing. Rename go to something more descriptive so others know exactly what the method does, maybe something like getRealCityTemp

event.preventDefault()
console.log(`Getting lat/lon and weather for city ${state.city}`);
getLatAndLon(state.city)
.then((latLon) => getWeatherKelvin(latLon.lat, latLon.lon))
.then((temp) => {
state.temperature = state.isF ? convertKtoF(temp) : convertKtoC(temp);
colorChange();
});
};

const changeSky = (event) => {
const inputSky = document.getElementById('skySelect');

const bg = document.getElementsByTagName('body')[0];

Choose a reason for hiding this comment

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

We write code for other humans to read, even though bg is shorthand for background I would still name this variable background or skyColor to make it clear what this variable is.


switch (inputSky.value) {
case 'Lover':
bg.style.background = 'url("assets/lover.jpeg")';
break;
case 'rep':
bg.style.background = 'url("assets/rep backup.jpeg")';
break;
case '1989':
bg.style.background = 'url("assets/1989 sky.jpeg")';
break;
case 'Red':
bg.style.background = 'url("assets/red.jpeg")';
break;
case 'Speak Now':
bg.style.background = 'url("assets/speak now.jpeg")';
break;
case 'fearless':
bg.style.background = 'url("assets/fearless.jpeg")';
break;
Comment on lines +94 to +111

Choose a reason for hiding this comment

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

Check out the logic on lines 94-111, see how similar the branches are of the switch statement. We look for some input value and then pick a background accordingly. What if we had a list of objects that we could iterate through to find the values. We could set up something like

const SKY_BACKGROUND = [ 
  { input: 'Speak Now', background: 'url("assets/speak now.jpeg")'}, 
  { input: 'fearless', background: 'url("assets/fearless.jpeg")}
];

Then your method would iterate through SKY_BACKGROUND and find the first record that matches the input value, then use it as the source to pick which background to render, which would help make the function a bit more concise.

This could accommodate a scenario where you might have even more backgrounds in the future, which would prevent you from having a really long switch statement

}

bg.style.backgroundSize = 'cover';
};

const colorChange = () => {

Choose a reason for hiding this comment

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

Consider renaming this method to be more descriptive like setColorAndOutfit

let tempColor;
let currentOutfit;
let lyric;

let temp = state.temperature;
if (!state.isF) {
temp = convertCtoF(temp);
}

if (temp >= 80) {
tempColor = 'red';
currentOutfit = 'assets/80.jpeg';
lyric = 'Shake it Off';
} else if (temp >= 70) {
tempColor = 'orange';
currentOutfit = 'assets/75.jpeg';
lyric = 'It feels like a perfect night to dress up like hipsters';
} else if (temp >= 60) {
tempColor = 'yellow';
currentOutfit = 'assets/50.jpeg';
lyric = 'Autumn leaves falling down like pieces into place';
} else if (temp >= 50) {
tempColor = 'green';
currentOutfit = 'assets/60.jpg';
lyric = 'There is something about the rain';
} else {
tempColor = 'teal';
currentOutfit = 'assets/49.jpeg';
lyric = 'Forever Winter if you go';
}
Comment on lines +127 to +147

Choose a reason for hiding this comment

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

Like I mentioned in your changeSky method, you can use a list here to contain objects that have the color, outfit and lyric values for each temperature range. Then your method would iterate through the constant list and find the first record that has an upper bound higher than our temperature, then use it as the source of picking the color/outfit/lyric.


const newOutfit = document.getElementById('outfits');
newOutfit.src = currentOutfit;

const temperature = document.getElementById('degrees');
temperature.style.color = tempColor;
temperature.textContent = String(Math.round(state.temperature));

const lyricChange = document.getElementById('lyrics')
lyricChange.textContent = lyric
};

const switchCAndF = (event) => {
state.temperature = state.isF
? convertFtoC(state.temperature)
: convertCtoF(state.temperature);
state.isF = !state.isF;

const unit = document.getElementById('unit');
unit.textContent = state.isF ? '°F' : '°C';

colorChange();
};
Comment on lines +160 to +170

Choose a reason for hiding this comment

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

👍


const registerEventHandlers = (event) => {
colorChange();

const increaseButton = document.querySelector('#right');
increaseButton.addEventListener('click', increaseTemp);

const decreaseButton = document.querySelector('#left');
decreaseButton.addEventListener('click', decreaseTemp);

const resetButton = document.querySelector('#reset');
resetButton.addEventListener('click', reset);

const goButton = document.querySelector('#search');
goButton.addEventListener('submit', go);

const newSky = document.querySelector('#skySelect');
newSky.addEventListener('change', changeSky);

const city = document.querySelector('#city');
city.addEventListener('input', handleCityEntered);

const fOrC = document.querySelector('#fOrC');
fOrC.addEventListener('click', switchCAndF);
};

document.addEventListener('DOMContentLoaded', registerEventHandlers);
Loading