@agusc01
Posted
a person uses media queries to change properties if they don't change the properties media queries are not used. I mention it because there is unnecessary code like
.body{ background-image\: url(./bg-desktop.svg); background-repeat: no-repeat; }
then @media screen and (min-width: 768px) { body { background-image: url(./bg-desktop.svg); .... background-repeat: no-repeat; } ..... }
And you know what...again....
@media screen and (min-width: 375px) and (max-width: 767px) { body { .... background-image: url(./bg-mobile.svg); background-repeat: no-repeat; } ..... }
the properties and values did not change why would you write the same thing again??? and that is not the only case you repeat this pattern more than once. Today it might be nothing, but in the future with a big project you will go crazy
It is not good that you use a class for a <div> and an <a> I do not see why they would share the class ".icons"
I recommend that you put .icons-box{ text-decoration:none} to remove the underline
Try to use classes as much as possible and avoid ID's
Writing javascript in the HTML is absolutely no good, neither is putting CSS in your HTML (like inline styling). Now it's nothing but in bigger project , it's necessary split it
First you should solve the previously mentioned problem of the "icons" class being in one <div> and in 3 <a>. I would put something like this
<div class="icon"> <a class="icon__link" href="www.facebook.com" target="_blank"> <i class="icon__italic fab fa-facebook-f fa-lg" ></i> </a> <a class="icon__link" href="www.twitter.com" target="_blank"> <i class="icon__italic fab fa-twitter fa-lg" ></i> </a> <a class="icon__link" href="www.instagram.com" target="_blank"> <i class="icon__italic fab fa-instagram fa-lg" ></i> </a> </div>javascript-file
const iconLinks = document.querySelector(".icon__link") iconLinks.forEach(link => { link.addEventListener("mouseover",()=>{ link.classList.add("mouse-over-colour") }) link.addEventListener("mouseleave",()=>{ link.classList.remove("mouse-over-colour") }) });
css-file .mouse-over-coulour{ color='#E882E8'; }
the default color (white) it'll on the class ".icon__italic" for example
And you have to create folders to separate contents . You really need it
Keep coding . ! No one was born knowing
@juanitatime
Posted
Hi @agusc01, I appreciate the thorough review of my code. I'll try implementing them and see if it doesn't disrupt anything. Will take note of the tips you gave and get back to you incase I find problems. Thanks!
@juanitatime
Posted
hi @agusc01,
I've made the changes to most of the elements you suggested. You can check the file here: https://github.com/juanitatime/Hubble-Landing-Page.github.io/tree/new-branch.
RE Repeating properties: There are two different background images for the mobile and desktop which was why I had to copy them again in different media queries. I have removed the 1st property with background-image already. Although I had to repeat the rest since they had to be styled differently.
RE Icons: That was the only way I figured how to change the color which was why there was Inline CSS. I followed your solution and it worked well.
Re Javascript: I am having trouble with making the links/icons hover using the Javascipt you gave since I am not that good at that yet. I've put the way I know how in a comment but it doesn't work either. Can you help me how to do it?
The new changes are in a different branch - contents are in a different folder this time. :D
Your help will be very much appreciated. Will definitely acknowledge you in my readme. :) Thank you!!
@agusc01
Posted
Hi @juanitatime
RE repeating properties. You rigth there are two different background BUT background-repeat was wrote 3 times there is no excuse LOL. I know copy and paste something goes wrong, it even happens to me. xD . You still have a lot of things to delete it on your new-branch
Re icons: great !
Re Javascript: If you don't feel comfort with my way to write javascript don't worry , it just practice . I put a easier example just with css (using :hover instead of javascript)
RE different branch : I see, that you have different folder. It's better !
More things to improve: In the next link you can see comments a code changes. https://onlinegdb.com/8MTBNbh6v
This have three tabs ("index.html","script.js", and "style.css") go into each tab and copy to then paste into your project
Have a good day !
@juanitatime
Posted
@agusc01 Thank you so much Agustin for your time and effort in helping me out with this. I definitely learned a lot from you. Yes, I'm starting to study about BEM now.
