Local Weather challenge - feedback

[carlostahira]

Hello everyone!

I have just finished my Local Weather challenge. I will be glad if someone could give a feedback for me.

https://codepen.io/carlostahira/full/LmYOZw/

I used the following things to do this challenge:

Javascript:

• navigator.geolocation
• LocalStorage .getItem() and .setItem()
• Number .isInteger() and .toFixed()
• JSON .parse() and .stringify()
• parseInt() and parseFloat()
• jQuery:
  – .ajax()
  – .fadeIn()
  – .find()
  – .each()

CSS/SASS

• Mobile First
• BEM
• mixin to create toggle button
• mixin to create loader
• animation (for loader)
• selectors :first-child and :last-child
• display Grid
• background position technique

Thanks!

[borisyordanov]

1. Any reason for using jQuery in this project? You don’t seem to be utilising it much
2. Your mixing ES5 and ES6 a lot, i see const, let, var, normal functions, arrow functions. Use ES6 more consistantly
3. You do a lot of dom queries when you functions are executed. Perhaps do them once when the app loads and not every time an action happens?
4. Overall you declare a lot of variables in your functions. Perhaps some of them can be global?
5. I’m happy to see that you added comments to your JS, but they could be better. It’s obvious that the $(document).ready() function is executed when the document is ready. It’s not immediately apparent what the geo location/local storage stuff are for.

Very impressive work!

[sorinr]

He’s using only jquery . i’dont see any arrow functions in his js code.
As for the dom queries you are right. @carlostahira you should try caching your dom like var title = $('.weather__title'); and use the vars

[borisyordanov]

ES6 variable syntax would be better though

[carlostahira]

Hello @borisyordanov @sorinr.

First of all, thanks for your feedbacks and sorry for delay. I refactored my challenge, check again if you could.

@borisyordanov

Any reason for using jQuery in this project? You don’t seem to be utilising it much

Yes, I have been studying only vanilla and I realized that I need to practice a little of jQuery, so I seized the opportunity to do it.

@borisyordanov

You do a lot of dom queries when you functions are executed. Perhaps do them once when the app loads and not every time an action happens?

@sorinr

As for the dom queries you are right. @carlostahira you should try caching your dom like var title = $(‘.weather__title’); and use the vars

Yeah, thanks for recomendation. I have already changed the code. Please, check my challenge again if possible

@borisyordanov

Overall you declare a lot of variables in your functions. Perhaps some of them can be global?

Except pieces of code where I did DOM queries I did not find other variables where the better choice is to become them global and give up encapsulation. Can you show me the exact part of my code to do it if still exists?

@borisyordanov

I’m happy to see that you added comments to your JS, but they could be better. It’s obvious that the $(document).ready() function is executed when the document is ready.

On previous block’s code I put a comment line before to explain it. I just tried to keep this pattern. But you are right, in that case was so obvious.

@borisyordanov

It’s not immediately apparent what the geo location/local storage stuff are for.

I realize that navigator geolocation is too slow and I solve it using localStorage:

• First I check if there are coordinates stored. If true, I immediately run my code to show the result
• For all request I always get geolocation’s user, around it and and compare with old coordinates.
• If the coordinates are different, save new coordinates and run the code again.

So, it solve 2 problems:

1. High load time to show the weather condition for user. It will be slow only on the first time
2. Update weather condition values automatically if the user changed location compared the last time he visited the page (a user visited the page, close the notebook, moved to other country and revisited the page)