Weather Project - Max Iriye, Natasha Razmadze

[NataliaRaz]

No description provided.

[yangashley]

Nice job on weather-report, team!

[yangashley]

I've seen a couple other examples of folks using span tags for these buttons. Did we provide an example with this kind of syntax? I'm guessing we did...

Personally, I'd prefer the more semantic button elements here for accessibility purposes since span is kind of 'meaningless'.

[yangashley]

Nice job using a constant variable to reference a string which makes your code more readable and also in the long run more maintainable because we can look at the top of the file for the variable to update the string (like for example, if the port number changes and we have to update the endpoint). Then we could avoid combing through lots of code to make a small update.

Technically, the string referred to by this variable is not an API key, but an base endpoint.

Suggested change

	const API_KEY = 'http://localhost:5000';
	const BASE_URL = 'http://localhost:5000';

[yangashley]

The logic between these 2 methods are pretty similar. Consider combining them into one method and having some logic to determine if you need to increment or decrement the weather.

[yangashley]

You could create an object that has color data, landscape data, etc for each temperature. Then you could use a loop to go through the object instead of having if/else if/else, which would make the function more concise and look something like:

const TEMP_DATA = {
  90: {
    color: 'red',
    landscape: '🌵__🐍_🦂_🌵🌵__🐍_🏜_🦂'
  }, 
  70: { // other data, etc...}
}

for (const [temp, values] of Object.entries(TEMP_DATA)) { 
  if (currentTemp >= temp) { // this condition needs refactoring, but you get the idea
  state.temp.className = value.color; 
  state.landscape.textContent = value.icon; 
  } 
}

Then you can move away from having a brittle implementation that can be tricky to make changes to.

[yangashley]

Like my comment above, what if you looped over an object with data so you could move away from using if/else if/else?

[yangashley]

Nice job using a data structure to get the value instead of using if/else if/else!

[yangashley]

There are 3 other places that repeat setTempColor and setLandscape. You could create a helper like setDisplayForTemperature that invokes these 2 methods. It's simple enough logic that I think it's ok as is, but also introducing setDisplayForTemperature would make the code a little more self-documenting.

[yangashley]

Could also separate these 2 chunks of logic so you have a helper function getCityLatLon and getTempFromLatLon.

This does work, but imagine the logic for getting live temperature changes. Instead of digging through one larger function, it would be easier to refactor a single-responsibility helper function.

[yangashley]

Nice job putting all the event handler registration logic in one place, this makes it easier to follow along and I know I only need to look in one place to find something related to registering a handler.

I'd prefer that the logic on lines 93-109 be put in a helper function, like registerEventHandlers and then you create an onLoaded helper function that executes all the code that needs to happen on page load.

The below example makes it easier to understand that there is code that should execute when the page first loads.

const registerEventHandlers = () => {
  const increaseButton = document.querySelector('#increaseTempControl');
  increaseButton.addEventListener('click', increaseTemp);

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

  const cityField = document.querySelector('#cityNameInput');
  cityField.addEventListener('change', cityInput);

  const getTempButton = document.querySelector('#currentTempButton');
  getTempButton.addEventListener('click', setTempFromCity)

  const skyElement = document.querySelector('#skySelect');
  skyElement.addEventListener("change", setSky)

  const cityReset = document.querySelector('#cityNameReset');
  cityReset.addEventListener('click', resetCity);
};

const onLoaded = () => {
  registerEventHandlers();
};

onLoaded();

[yangashley]

document.querySelector('#headerCityName'); appears in your code 3 times. While it's not a ton of repetition, it still is three function calls and these calls aren't complex, time intensive operations.

However, each time we type it, we could introduce a bug.

I'd prefer a global variable that can be referenced throughout the file or create an object with all the elements you'll need, like we did in our livecode: https://github.com/Ada-Activities/Pick-Me-Up-Pup/blob/c23-solution/src/index.js#L4-L10