Weather App - Project Feedback

Code Feedback
1

Hi folks!

So I just wrapped up the fcc weather app project. I’m not 100% satisfied with the design but it’s enough where I can move on and come back later. Any feedback on it would be very much appreciated, I went a little beyond the challenge scope by adding a time and date display and a couple of design features. The clock btw should (hopefully) update itself if you leave the app open. Meanwhile the background icon will change depending on the weather and the background color itself changes depending on the time of day.

One weird problem I’m experiencing and maybe someone can help me out…but I can’t get the temperature and weather icon to display in IE (of course IE is being a pain in the butt right?). I thought at first it was just the api itself, but the location and weather type description show up fine. It’s gotta be something in my script but I’m not sure what. I’ve tried running the app in IE both via codepen and my own local server and I’ll get errors in the console for codepen, and by that I mean errors on codepen’s end, like their code. Meanwhile the console on my local sever has no errors at all, so I’m at a bit of a loss as to what’s causing it.

Other than that I’m still tweaking the mobile display but it should be good to go. I have admit this was a really fun project to work on, really challenging and I learned alot!

2

Words in HTML attributes should be separated by dashes
Don’t use ids for styling, that’s what classesarefor
Rethink your “Browser style reset”, there’s an easier way to do what you want without listing every html tag, you’re using it in your first line of CSS
Format your code better
It’s nice to see ES6 but why aren’t you more consistent with it? I still see var and your anon functions aren’t arrow functions.
You’re storing a lot of variables and doing a lot of DOM queries in callbacks, perhaps move them to the global scope
It’s a good idea to learn jQuery, but i’d suggest forking this pen and trying to do it without any libraries. This will help you appreciate the benefits of using libraries and will help you understand how web pages work natively

3

Thanks for the feedback @borisyordanov, I wanted to comment on a few of your points:

My use of ids is because I want my elements to be unique (which ids should be), none of the ones I’m targeting are repeated and in that case I would use classes.

True, but I like the option of just deleting a selector from a list if I need to. Basically doing it like I have hasn’t done me wrong yet.

My use of let here and there is to denote one off variables. While technically I can use it for my globals, and actually I probably should be using const instead for some of them, I like var for its accessibility. As for not using arrow functions that just comes down to readability. I personally like the look of the old anon function, especially while I’m still learning all this. It helps to instantly be able to parse what I’m looking at.

Can you give me an example from my code?

Absolutely. But my use of jQ here was simply for speed of production. I wanted to focus mainly on working with the API since I’ve never done it before. I know how to target and manipulate DOM elements with vanilla JS so using jQ there to streamline it was a definite yes for me.

As for using it in my JSON call, I did a bit of research on how to do it via vanilla vs jQ and I kept seeing the same statements over and over again that “there’s no point in reinventing the wheel, doing it with jQ is way faster and easier, just use that.” So for that portion at least I’d like to go back at some point and try with JS, just to get a feel for it.

4

My use of ids is because I want my elements to be unique (which ids should be), none of the ones I’m targeting are repeated and in that case I would use classes.

I think you’re confusing JS dom queries with style cascading - https://codepen.io/boris-yordanov/pen/rKVyKm

My use of let here and there is to denote one off variables. While technically I can use it for my globals, and actually I probably should be using const instead for some of them, I like var for its accessibility. As for not using arrow functions that just comes down to readability. I personally like the look of the old anon function, especially while I’m still learning all this. It helps to instantly be able to parse what I’m looking at.

The general rule is use const, unless you need to use let:

If you don’t want to do that and you prefer ES5 that’s fine. Commit to a version of the language that works for you and be consistent with its use.

Can you give me an example from my code?
This is one thing that should be a const in the global scope:

// OpenWeather API key
let apiKey = "29839d41690f616b6fe945e578fe0a06";

Do any of these DOM elements change locaton during the life cycle of the page? If not, why do you query them every time the weather data is updated? Store them as global variables and change their text content when needed.

$("#lat").text(json.coord.lat);
$("#long").text(json.coord.lon);
$("#temp").text(fahrenheit);
$("#weathertype").text(json.weather[0].main);
$("#city").text(json.name);
$("#country").text(json.sys.country);

Absolutely. But my use of jQ here was simply for speed of production. I wanted to focus mainly on working with the API since I’ve never done it before. I know how to target and manipulate DOM elements with vanilla JS so using jQ there to streamline it was a definite yes for me.
As for using it in my JSON call, I did a bit of research on how to do it via vanilla vs jQ and I kept seeing the same statements over and over again that “there’s no point in reinventing the wheel, doing it with jQ is way faster and easier, just use that.” So for that portion at least I’d like to go back at some point and try with JS, just to get a feel for it.

Totally get it, this would make sense if you were making a real-world project. However, when you are learning you shouldn’t choose the easy way. Your goal is to learn something, not to save time. If something is trivial to you, sure - go ahead and cut corners, but until then try to do things yourself and not to rely on others’ code.

1 Like
5

I was replying to your original statement about using only classes for styling (CSS), unless you were referring to styling via JS? In that case it’s still the same principle that ids should only be used once, your example pen uses the same id twice in the HTML which is incorrect. As for adjusting a style via JS, I’d do something like this most likely:

document.getElementById("title");

Though document.querySelector("#title"); is totally fine too. They’re really the same thing except in the second instance you need the # to define it as a id and the first instance it’s implied.

Or where you referring to something else?

True, it could be. But since I was only referring to the API key once and only in the function it was used in, I figured let was the way to go. Again, it’s a one shot variable, using let for it made sense to me.

Because it cuts down on code in this case. Here I’m targeting and changing them when needed all in one line via jQ. Could I have done the same thing via JS and storing globals? Totally, and I know how to do that too, but again my focus here was just on learning how to work with an API in general.

That being said, I’m definitely going to go back and practice the JSON api call portion sans a library. But as for straight up DOM manipulation, I’m getting it out of the way quickly to hone in on the stuff I don’t know. Does that make sense?

I totally get and appreciate your emphasis on learning the bare-bones, basics on how to do things, it’s the same thing I tell other campers. But I think you’re coming at my project thinking I’m a complete beginner, which I am in some cases but not in others.

Oh! I wanted to thank you for those variable links by the way. And I also get where you’re coming from about mixing ES5 and 6. So far my mixing of the two have been very slight, and to me isn’t really an issue, but I can see ping-ponging from one to the other with something like functions could definitely be confusing to anyone else reading my code. And I’ll totally keep that in mind.