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/Hailey M. #74

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

Sharks/Hailey M. #74

wants to merge 17 commits into from

Conversation

HaileyMatz
Copy link

No description provided.

Copy link

@audreyandoy audreyandoy left a comment

Choose a reason for hiding this comment

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

Hi Hailey! Your project meets all requirements and looks great. Not only that but you made your own designs come to life!!!!!

I left some comments on how to refactor. Great work on making your design responsive, however, I would consider adjusting the text size so that it doesn't shrink too much.

Other than that, fantastic work Hailey :)

Grade: 🟢

Comment on lines +1 to +7
const state = {
location: '',
lat: 0,
lon: 0,
temp: 32,
sky: '',
};

Choose a reason for hiding this comment

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

👍 Nice work creating a state object. This is very similar to how 'state' aka global data is shared across an entire app and we'll see React use a similar pattern of storing data in objects.

Comment on lines +9 to +26
const getLonLat = () => {
axios
.get('https://weather-report-proxy-server.herokuapp.com/location', {
params: {
q: state.location,
format: 'json',
},
})
.then((response) => {
console.log(response);
state.lon = response.data[0].lon;
state.lat = response.data[0].lat;
getLocationWeather();
})
.catch((error) => {
console.log(error);
});
};

Choose a reason for hiding this comment

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

👍 . Good work with error handling and using console.log to confirm the behavior of your get request. However, the nature of PR's is to provide code you eventually want your team to have. In this case, we can keep the console.logs local to our project and remove them for PRs.

Comment on lines +28 to +49
const getLocationWeather = () => {
axios
.get('https://weather-report-proxy-server.herokuapp.com/weather', {
params: {
lat: state.lat,
lon: state.lon,
},
})
.then((response) => {
condition = response.data.current.weather[0].main;
validateSkyCondition(condition);
const kelvin = response.data.current.temp;
const fTemp = Math.floor(((kelvin - 273.15) * 9) / 5 + 32);
state.temp = fTemp;
document.getElementById('rangeValue').textContent = fTemp;
document.getElementById('range').value = fTemp;
changeColors();
})
.catch((error) => {
console.log('error');
});
};

Choose a reason for hiding this comment

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

Nice work using the lat and lon stored in state to pass into the API call.

Choose a reason for hiding this comment

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

Also nice work in converting kelvin to Fahrenheit!

Comment on lines +51 to +55
function clickBtn(event) {
const city = document.getElementById('city').value;
state.location = city;
getLonLat();
}

Choose a reason for hiding this comment

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

The name clickBtn is very generic, we'll often have several buttons on a web app. A function name should describe what the function does! In this case, we're setting the location in state and then getting the lat and longitude. Perhaps we might want to change this to "setCity" or "setLocation".

Comment on lines +67 to +87
const validateSkyCondition = (condition) => {
console.log(condition);
if (condition === 'Clear') {
state.sky.value = 'sunny';
document.getElementById('skySelect').value = '#7496C9';
} else if (
condition === 'Rain' ||
condition === 'Drizzle' ||
condition === 'Thunderstorm'
) {
state.sky.value = 'rainy';
document.getElementById('skySelect').value = '#356098';
} else if (condition === 'Clouds' || condition === 'Haze') {
state.sky.value = 'cloudy';
document.getElementById('skySelect').value = '#557BB1';
} else if (condition === 'Snow') {
state.sky.value = 'snowy';
document.getElementById('skySelect').value = '#234772';
}
skyChange();
};

Choose a reason for hiding this comment

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

I LOVED this enhancement, Hailey! It's so cool to see you develop an app based on your very own designs. I remember you being nervous about this feature during a 1:1 and I'm so proud of you for making it work ✨ YOU did that ❤️ 🔥 !!!

Comment on lines +118 to +147
const changeRangeS = () => {
const input = document.getElementById('range');
input.addEventListener('input', (event) => {
if (input.value >= 80) {
document.getElementById('city-circle').style.background = '#EA9A9F';
document.getElementById('temp-circle').style.background = '#EA9A9F';
document.getElementById('sky-circle').style.background = '#EA9A9F';
document.getElementById('weather').style.color = '#EA9A9F';
} else if (input.value >= 70) {
document.getElementById('city-circle').style.background = '#E8BCC3';
document.getElementById('temp-circle').style.background = '#E8BCC3';
document.getElementById('sky-circle').style.background = '#E8BCC3';
document.getElementById('weather').style.color = '#E8BCC3';
} else if (input.value >= 60) {
document.getElementById('city-circle').style.background = '#E5DDE7';
document.getElementById('temp-circle').style.background = '#E5DDE7';
document.getElementById('sky-circle').style.background = '#E5DDE7';
document.getElementById('weather').style.color = '#E5DDE7';
} else if (input.value >= 50) {
document.getElementById('city-circle').style.background = '#CECEE0';
document.getElementById('temp-circle').style.background = '#CECEE0';
document.getElementById('sky-circle').style.background = '#CECEE0';
document.getElementById('weather').style.color = '#CECEE0';
} else if (input.value < 50) {
document.getElementById('city-circle').style.background = '#AEBFDD';
document.getElementById('temp-circle').style.background = '#AEBFDD';
document.getElementById('sky-circle').style.background = '#AEBFDD';
}
});
};

Choose a reason for hiding this comment

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

There's a lot of repetition in this function. Notice how in each conditional, you are setting the same three elements to the same background color? We can DRY up our code by using the conditionals to instead change the value of a variable and THEN set that color to the elements backgrounds.

const changeRanges = () => {
  const input = document.getElementById('range');
  input.addEventListener('input', (event) => {
    let color = ''
    if (input.value >= 80) {
      color = '#EA9A9F';
    } else if (input.value >= 70) {
      color = '#E8BCC3';
    } else if (input.value >= 60) {
      color = '#E5DDE7';
    } else if (input.value >= 50) {
      color = '#CECEE0';
    } else if (input.value < 50) {
      color = '#AEBFDD';
    }
    document.getElementById('city-circle').style.background = color;
    document.getElementById('temp-circle').style.background = color;
    document.getElementById('sky-circle').style.background = color;
  });
};

Comment on lines +89 to +116
const changeColors = () => {
const rangeVal = document.getElementById('range');
if (rangeVal.value >= 80) {
document.getElementById('city-circle').style.background = '#EA9A9F';
document.getElementById('temp-circle').style.background = '#EA9A9F';
document.getElementById('sky-circle').style.background = '#EA9A9F';
document.getElementById('weather').style.color = '#EA9A9F';
} else if (rangeVal.value >= 70) {
document.getElementById('city-circle').style.background = '#E8BCC3';
document.getElementById('temp-circle').style.background = '#E8BCC3';
document.getElementById('sky-circle').style.background = '#E8BCC3';
document.getElementById('weather').style.color = '#E8BCC3';
} else if (rangeVal.value >= 60) {
document.getElementById('city-circle').style.background = '#E5DDE7';
document.getElementById('temp-circle').style.background = '#E5DDE7';
document.getElementById('sky-circle').style.background = '#E5DDE7';
document.getElementById('weather').style.color = '#E5DDE7';
} else if (rangeVal.value >= 50) {
document.getElementById('city-circle').style.background = '#CECEE0';
document.getElementById('temp-circle').style.background = '#CECEE0';
document.getElementById('sky-circle').style.background = '#CECEE0';
document.getElementById('weather').style.color = '#CECEE0';
} else if (rangeVal.value < 50) {
document.getElementById('city-circle').style.background = '#AEBFDD';
document.getElementById('temp-circle').style.background = '#AEBFDD';
document.getElementById('sky-circle').style.background = '#AEBFDD';
}
};

Choose a reason for hiding this comment

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

Hm... changeColors and changeRangeS are very similar. There's probably a way of combining both of them!

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