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

Panthers - Billie Betts #91

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

Panthers - Billie Betts #91

wants to merge 7 commits into from

Conversation

lbbetts
Copy link

@lbbetts lbbetts commented Dec 14, 2022

No description provided.

index.html Outdated
Comment on lines 65 to 69
</div>
<div class="grid-item-field">
<p id="temp_field">🌷</p>
</div>
</div>

Choose a reason for hiding this comment

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

Rather than hard-coding a value for UI elements (which just happens to match the initial value backing the control), I like to leave the initial markup blank, and update the UI on first load from the initial backing values in the JS code. So like here, leave off the emoji, then when the document loads, update the UI in the JS to show the initial sky and landscape value. This would protect us from changing the default starting JS value, but having it be out of sync with what the UI is showing.

I would tend to do this for any of the configurable elements (temperature, ground, sky, city).

Comment on lines +16 to +29

.header {
text-align: center;
font-family: "American Typewriter";
font-size: 25px;
}

.city-header {
font-family: "Gill Sans";
font-size: 45px;
}

.grid-container {
display: grid;

Choose a reason for hiding this comment

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

Amazing work with styling 👍🏾 I would suggest adding media queries to account for all types of screen sizes


// CONDITIONS

let selectConditions = document.querySelectorAll('input[type="radio"]');

Choose a reason for hiding this comment

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

Nice job using this radio, I'll allow it even though there's an issue with wave 05 (selecting the sky), the requirements specify that a user should select from a dropdown element.

Comment on lines +39 to +40
const HOW_YA_FEELIN = ['🥶', '😮‍💨', '😄', '😎', '😅', '🥵', '🤔'];
const HILLS_ARE_ALIVE = ['🧊', '🌲', '🌷', '🌻', '🌴', '🔥', '🌎'];

Choose a reason for hiding this comment

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

Cute emojis! 😁

Comment on lines +53 to +68
const updateTempies = (adjustedTemp) => {
let tempSetting = 6;

if (adjustedTemp > 99) {
tempSetting = 5;
} else if (adjustedTemp > 79) {
tempSetting = 4;
} else if (adjustedTemp > 65) {
tempSetting = 3;
} else if (adjustedTemp > 39) {
tempSetting = 2;
} else if (adjustedTemp > 14) {
tempSetting = 1;
} else if (adjustedTemp <= 14) {
tempSetting = 0;
}

Choose a reason for hiding this comment

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

Nice work with this conditional Billie!

Comment on lines +72 to +73
showField.innerHTML = HILLS_ARE_ALIVE[tempSetting];
showYou.innerHTML = HOW_YA_FEELIN[tempSetting];

Choose a reason for hiding this comment

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

Nice approach to this array 🥳

Comment on lines +85 to +97
const handleAdd = () => {
currentTemp++;
shownTemp.innerHTML = currentTemp;

updateTempies(currentTemp);
};

const handleSub = () => {
currentTemp--;
shownTemp.innerHTML = currentTemp;

updateTempies(currentTemp);
};

Choose a reason for hiding this comment

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

Great function naming 👍🏾

Comment on lines +143 to +170
const modifyWeather = (weatherData) => {
let relevantData = [weatherData['main'], weatherData['weather']];
let defTempCond = [];

let unmodifiedTemp = relevantData[0]['temp'];
let tempF = ((unmodifiedTemp - 273.15) * 9) / 5 + 32;
let modifiedTemp = tempF.toFixed(0);
defTempCond.push(modifiedTemp);

let condID = Number(relevantData[1][0]['id']);
let idToCond = 'sunny';

if (condID > 800) {
idToCond = '⛅';
} else if (condID === 800) {
idToCond = '✨';
} else if (condID > 700) {
idToCond = '⛅';
} else if (condID > 599) {
idToCond = '☃️';
} else {
idToCond = '🌈';
}

defTempCond.push(idToCond);
return defTempCond;
};

Choose a reason for hiding this comment

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

Consider moving this outside of the function to separate concerns but great work with this approach 🏁

Comment on lines +106 to +134
.get('http://127.0.0.1:5000/location', {
params: {
q: currentCity,
format: 'json',
},
})
.then((response) => {
locationData = response.data;

const modifyLocation = (locationData) => {
let latlon = [];
let focusLocation = locationData[0];

let focusLat = Number(focusLocation['lat']);
let adjustLat = focusLat.toFixed(2);
latlon.push(adjustLat);

let focusLon = Number(focusLocation['lon']);
let adjustLon = focusLon.toFixed(2);
latlon.push(adjustLon);

return latlon;
};

modifiedLocData = modifyLocation(locationData);

axios
.get('http://127.0.0.1:5000/weather', {
params: {

Choose a reason for hiding this comment

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

Consider breaking this function up a bit. It's taking on a lot responsibilty. Whenever we see a function that is more than 10 lines long we should consider seprating concerns. This is completely preferential but just a note. More on this here:

https://dmitripavlutin.com/the-art-of-writing-small-and-plain-functions/

https://medium.com/@janakachathuranga/coding-best-practices-javascript-write-small-functions-7d2567bd6328

Copy link

@ameerrah9 ameerrah9 left a comment

Choose a reason for hiding this comment

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

Great work, Billie! 🥳

Thank you for your patience as we catch up on grading. Nice work! The HTML is structured well and the JS is clear and well-factored. I did have a laugh at the emoji choices 🥳 This project is a Green. 🟢

Keep it up 💯

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