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

Add files via upload #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Ashbk32
Copy link

@Ashbk32 Ashbk32 commented Nov 22, 2019

HW 4
I had numerous issues trying to commit it and send it. I wasn't able to get it working tried multiple ways but I know I'm not confident in my coding at all. After trying to see the pattern I couldn't figure it out

when city is entered display results should include city name, current temp, weather description, min temp,max temp.

Once city is picked results should display but also change when adding an if statement to check if the weather is either above 85 degree or below 40.*/
$(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ashbk32 be careful re: indentation. It appears to be off throughout the code here in app.js. This makes it very difficult to scan and troubleshoot.

Once city is picked results should display but also change when adding an if statement to check if the weather is either above 85 degree or below 40.*/
$(function () {
$('#search').click(() => {
event.preventDefault()
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ashbk32 this should be inside of an event handler such as a .submit() see the solution for the way it should be structured: https://github.com/jsd20191008/weather-api-app-solution/blob/master/js/main-async-await.js#L2


// note added async to function

async function getWeather (searchTerm) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ashbk32 looks like you're attempting to declare this function inside of the searchCity() function. You generally want to avoid that because it complicates how the functions will get called.

// note added async to function

async function getWeather (searchTerm) {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ashbk32 nice job here, it looks like the body of your Async / Await function is structured correctly. However, due to the comment above, getting this code to run will be difficult.

@@ -0,0 +1,32 @@
<!DOCTYPE html>
<html lang="en">
<head>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ashbk32 nice updates to the html

@kareemgrant
Copy link
Contributor

kareemgrant commented Dec 3, 2019

@Ashbk32 Great effort here! The main issue I'd like for you to focus on is indentation. It sounds trivial but it is key to being able to troubleshoot code when things aren't working. Incorrect indentation leads to avoidable syntax errors which prevent your code from running and therefore makes it hard to fix things when they are broken. Code will always break, the key is being able to find the errors when they occur and proper indentation is a huge part of that.

Take a look at the solution for more context. I suggest you pick one (I've provided .js files for three approaches - $.ajax(), standard promises & async/await) and walk through the code line by line and try to see if you can "talk through" what is happening in your own words. Reading other people's code is one of the best ways to learn code.

solution: https://github.com/jsd20191008/weather-api-app-solution/blob/master/js/main-async-await.js#L2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants