My overall impression?
The good:
You figured out how to do AJAX requests (if you didn’t on the quote project). A much needed skill, and this project will only help you understand it better.
You got it working. No really, this is a tough project for a beginner, so kudos for finishing a working version. This project stumped me for quite a while, and it’s stumped a lot of others too.
Temperature slider is nifty. I like that.
The bad:
Fonts. You are using two totally different types of fonts. A script for the title, and a big sans-serif for the text. I don’t know what to say, but I really like the way the font looks on the slider, but it just looks to bulky in the middle.
String concatenation.
var myWeather =
"https://fcc-weather-api.glitch.me/api/current?lat=" +
myLan +
"&" +
"lon=" +
myLon;
Use template literals:
let myWeather = `https://fcc-weather-api.glitch.me/api/current?lat=${myLan}&lon=${myLon}`;
var. Let and const keywords are preferred:
let myLan = position.coords.latitude;
let myLon = position.coords.longitude;
Long if chain:
if (low === "clouds") {
$("#backPicture").css(
"background",
"url( http://fs1.directupload.net/images/180329/d9rgbk9f.jpg)"
);
$("#backPicture").css("background-repeat", "no-repeat");
$("#backPicture").css("background-size", "cover");
} else if (low === "rain") {
$("#backPicture").css(
"background",
"url( http://fs1.directupload.net/images/180329/nudfcojz.jpg)"
);
$("#backPicture").css("background-repeat", "no-repeat");
$("#backPicture").css("background-size", "cover");
} else if (low === "snow") {
$("#backPicture").css(
"background",
"url( http://fs1.directupload.net/images/180329/ru8h6i5g.jpg)"
);
$("#backPicture").css("background-repeat", "no-repeat");
$("#backPicture").css("background-size", "cover");
} else if (low === "thunderstorm") {
$("#backPicture").css(
"background",
"url( http://fs1.directupload.net/images/180329/qd9ftdff.jpg)"
);
$("#backPicture").css("background-repeat", "no-repeat");
$("#backPicture").css("background-size", "cover");
} else if (low === "drizzle") {
$("#backPicture").css(
"background",
"url( http://i63.tinypic.com/33eubms.jpg)"
);
$("#backPicture").css("background-repeat", "no-repeat");
$("#backPicture").css("background-size", "cover");
} else {
return;
}
}
A switch statement would probably be better there.
The Ugly:
Speed. Clear your cache and cookies and reload your page. You should see what I’m talking about. It takes several seconds for the image to load. Then it asks for your location and waits another second to get the data. Once the data arrives, it takes another few seconds to get the new background if it is needed.
First step, get rid of the document.ready. You are waiting for the slow image to load before asking if they even want to give location. First thing they should get is a request to get data. That way, the user doesn’t have to wait for the image to load, get data, and then wait for another image to load.
Second tip. Resize your image. There is no reason for me to load a 5854px by 3892px image. You are going to make your mobile users go bankrupt, especially since they may have to load 2 of these things. The largest size you should have for a background is normal 1080px. Even if your monitor is 4k, it doesn’t matter.
Code formatting. An absolute mess:
$( document ).ready(function(){
if (navigator.geolocation) {
navigator.geolocation.getCurrentPosition(function(position) {
var myLan = position.coords.latitude;
var myLon = position.coords.longitude;
var myWeather = "https://fcc-weather-api.glitch.me/api/current?lat="+myLan+"&"+"lon="+myLon ;
$.ajax({
url: myWeather, success: function (result) {
$("#data").text(result.name);
$("#current").text(result.sys.country);
var currentWeather = result.weather[0].main;
$("#condition").text(currentWeather);
var currentIcon = "" + result.weather[0]["icon"];
$("#iconWeather").attr("src", currentIcon);
$("#currentTmp").text(result.main["temp"] + "°");
$("#myonoffswitch").on("click", function(){
if(document.getElementById('myonoffswitch').checked){
var fahreheit = result.main["temp"]* 1.8 + 32;
$("#currentTmp").text(fahreheit + "°");
}else{
$("#currentTmp").text(result.main["temp"] + "°");
}
});
var low= currentWeather.toLowerCase();
if(low ==="clouds"){
$("#backPicture").css("background","url( http://fs1.directupload.net/images/180329/d9rgbk9f.jpg)");
$("#backPicture").css("background-repeat","no-repeat");
$("#backPicture").css("background-size",'cover');
}else if(low==="rain"){
$("#backPicture").css("background","url( http://fs1.directupload.net/images/180329/nudfcojz.jpg)");
$("#backPicture").css("background-repeat","no-repeat");
$("#backPicture").css("background-size",'cover');
}else if(low ==="snow"){
$("#backPicture").css("background","url( http://fs1.directupload.net/images/180329/ru8h6i5g.jpg)");
$("#backPicture").css("background-repeat","no-repeat");
$("#backPicture").css("background-size",'cover');
}else if(low ==="thunderstorm"){
$("#backPicture").css("background","url( http://fs1.directupload.net/images/180329/qd9ftdff.jpg)");
$("#backPicture").css("background-repeat","no-repeat");
$("#backPicture").css("background-size",'cover');
}else if(low ==="drizzle"){
$("#backPicture").css("background","url( http://i63.tinypic.com/33eubms.jpg)");
$("#backPicture").css("background-repeat","no-repeat");
$("#backPicture").css("background-size",'cover');
}else{
return;
}
}
});
});
}
});
There is a nice little dropdown button at the top right corner that will easily make your code look nice. Please take the time to properly indent your code as it shows that you care about what you are doing.
TL;DR
Resize images and try to improve page load speed.
Make your code formatted properly.
Try to adopt newer newer JavaScript features such as let, fetch(), promises, async functions, etc. They will really make your code a lot simpler.
You completed a challenging project, so you should be really proud of yourself.
Hope this criticism helps, good luck advancing to the next level!
Seems to work well. But watch out for your code formatting. You ought to have a consistent style for indenting your code, otherwise it is very difficult for other people to read. Try this:
Also, you have these lines inside every if else block:
$("#backPicture").css("background-repeat", "no-repeat");
$("#backPicture").css("background-size", 'cover');
If you are applying these rules every time, they are not conditional, so they don’t need to be inside an if else block. You can just write them once.
By the way, the final return statement in the if block is not needed. Return is used to exit from a function and return a value, not to exit from a conditional block.
I agree, speed loading = efficiency, use a different approach from images as a background, otherwise try to compress them.
Thanks you very much for your feedback, do u know a good website, where i get the new featers of java script ?
I just used google and read several articles and books for ES6 and ES7 features.
I did a lot of changes on my project, can u tell if it is fast now?