Hubble Landing Page

Community feedback

• 

  Agustin•340

  @agusc01Posted over 2 years ago

  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

  0

  ---

  

  juanitatime•20

  @juanitatimePosted over 2 years ago

  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!

  0

  

  juanitatime•20

  @juanitatimePosted over 2 years ago

  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!!

  0

  

  Agustin•340

  @agusc01Posted over 2 years ago

  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 !

  0

  

  juanitatime•20

  @juanitatimePosted over 2 years ago

  @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.

  1
• 

  abdou1981•310

  @abdou1981Posted over 2 years ago

  Not responsive to all screens

  0

  ---

  

  juanitatime•20

  @juanitatimePosted over 2 years ago

  Hi @abdou1981,

  May I ask which screen size you mean by not responsive? I've tried it again and it works fine to me.

  0

  

  abdou1981•310

  @abdou1981Posted over 2 years ago

  @juanitatime Screen less than 375px

  0
• 

  Aadvik•1,250

  @Aadv1kPosted over 2 years ago

  Congrats on your first challenge.

  1. You cannot "style" icons, unless they are SVG in which case you can directly edit their color or other tweaks.
  2. Adding a border is pretty straight forward border: 1px solid black
  3. To add a hover effect you can

  element {
    transition: all 1s ease; // this is to make hover smooth
  }
  
  element:hover {
    border: 1px solid black
  }
  
  

  0

  ---

  

  juanitatime•20

  @juanitatimePosted over 2 years ago

  Hi @Aadv1k,

  thanks for helping me out. I have tried that for the border but once I do border-radius, the icon gets resized and doesn't look the same.

  0

  

  Aadvik•1,250

  @Aadv1kPosted over 2 years ago

  @juanitatime You can't directly apply a border-radius on a image, you must put it in a parent element then apply the border-radius to the parent

  0

  

  juanitatime•20

  @juanitatimePosted over 2 years ago

  @Aadv1k Yes I believe that's what I did. I put the icon inside an anchor class. But when I now hover, there's a separate one for the icon and another for the border

  0