HTML / CSS | Results-summary-component

Community feedback

• 

  Alexej Kunz•60

  @Orchi1904Posted over 1 year ago

  Hello ellenliao95,

  I am proud and happy to hear that you stuck with the challenge and did not give up. This is so important! Also I am happy to see your result. I am also only a beginner but I try to help you with some feedback.

  • The first thing is, as you can see in the accessibility report, <li> elements must be contained in a <ul> or <ol> like so:

  <ul>
     <li>
        //Your code          
     </li>
  </ul>
  

  In the case of this challenge, I would not even use <li> elements but rather use <div> elements. Here is an example of how I would structure the HTML without the <li> elements:

  <div class="container" id="bg-1">
    <div class="container-left">
      <svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" fill="none" 
               viewBox="0 0 20 20">
               <path stroke="#F55" stroke-linecap="round" stroke-linejoin="round" stroke- 
                width="1.25"
                d="M10.833 8.333V2.5l-6.666 9.167h5V17.5l6.666-9.167h-5Z" />
      </svg>
      <p id="li-1" class="li-space">Reaction</p>
    </div>
    <div class="li-num-2">80
      <span class="li-num-3"> / 100<span>
    </div>
  </div>
  

  This should be done for every container inside the "right-side" div. It will clean up your HTML a bit and fix the above <li> errors. You also have to change your CSS after a bit:

  .container{
      list-style: none;
      display: flex;
      justify-content: space-between;
      border: 1px solid none; //delete this, it does not change anything
      align-items: center; //add this, it will center the items inside the container vertically
      border-radius: 15px;
      padding: 12px;
      width: 200px;
      margin: 20px auto;
  }
  
  .container-left{
      display: flex;
      justify-content: space-between; //delete this, because container-class already has space-between
      align-items: center; //add this, it will center the svg and p vertically
  }
  
  

  • I also saw that you are not using the "Hanken Grotesk" font everywhere even though it is used everywhere in the solution. You could fix this easily by adding this to the body-selector in your CSS:

  font-family: "Hanken Grotesk", sans-serif;
  

  All the other font-family properties you used on other classes can than be deleted.

  • Your button uses the linear-gradiant background even though this should only be seen when someone hovers over that button. This can also be fixed very quick:

  button{
      width: 250px;
      height: 50px;
      border: none;
      border-radius: 25px;
      background: hsl(224, 30%, 27%); //use this color normally
      color: hsl(0, 0%, 100%);
      font-size: 18px;
      cursor: pointer;
  }
  
  //use linear-gradiant background, when somone hovers over the button
  button:hover{
      background:
      linear-gradient(
          hsl(252, 100%, 67%), 
          hsl(241, 81%,  54%) ) ;
  }
  
  

  • Also your design is not responsive, meaning it might look bad when somone visits your website with a smartphone. You can make your design responsive by using media queries. Normally you start designing your website for mobile first and than you use media queries to design it for bigger screens but you can also do it the opposite way. For example you could write something like:

  @media screen and (max-width: 720px){
   //Your code for small screens here
  }
  

  Inside this media query you can write CSS that should be displayed when somone visits your site with a screen-width smaller than 720px. Here is a explaination of media queries: Explaination on YouTube

  I hope I could help you with my feedback! Happy coding!

  0

  ---

  

  Yingchu Liao•30

  @ellenliao95Posted over 1 year ago

  @Orchi1904 Hi, Alexej Kunz

  Thank you for your reply!! I had the following question cloud you please help me?

  In HTML:

  • Why <div> is better then <ul> and <li> ? it's because we can use fewer lines?

  In CSS:

  • Why did I need to add align-items: center; in .container? I tried to do it like you, but it's not different on right-site, but left-site cannot center anymore, only using text-align can fix it in the body (I also tried to align-item: center in the body it's not work)

  about font-family if I put a font-family in the body, how can I change the font-size?? I tried to change font-size in .bottom-bg p { } but didn't work, I had this issue before, then I found out it's because google fonts are already set. How do you fix it??

  thank you to tell me about the button part, I miss it! also about media queries, I didn't learn before. thank you for reading every line of my code so carefully, which is very helpful to me, I'm also it English beginner so if you cannot understand feel free to ask me!

  Have a great day!!

  1

  

  Yingchu Liao•30

  @ellenliao95Posted over 1 year ago

  @Orchi1904

  May I see your code? I cannot find it in your GitHub

  0

  

  Alexej Kunz•60

  @Orchi1904Posted over 1 year ago

  @ellenliao95 Sure, I will try to help you.

  Q: Why <div> is better then <ul> and <li> ? it's because we can use fewer lines?

  A: It is not really about the number of lines but I would rather use <div> in this case because you are not really creating a list in that case. Ultimately, the choice of which HTML element to use should be based on the content and structure of the web page being created and in my opinion a <div> here fits better than <li>.

  Q: Why did I need to add align-items: center; in .container? I tried to do it like you, but it's not different on right-site, but left-site cannot center anymore, only using text-align can fix it in the body (I also tried to align-item: center in the body it's not work)

  A: It is strange that align-items: center; did not work in your case because it worked after I cloned the project and tried it out on my machine. Normally align-itmes: center; centers the items inside the container vertically. The problem was that your Icons were not centered after changing the HTML so you have to add this.

  Q: about font-family if I put a font-family in the body, how can I change the font-size?? I tried to change font-size in .bottom-bg p { } but didn't work, I had this issue before, then I found out it's because google fonts are already set. How do you fix it??

  A: You should be able to change the font-size with google fonts just like with every other font.

  PS: I created a pull request with the changes I told you in the feedback above. You can accept it if you want and there you can see in your code what I meant exactly.

  Have a great day too :)

  0

  

  Alexej Kunz•60

  @Orchi1904Posted over 1 year ago

  @ellenliao95

  Here is the link to my project

  Be aware that I used the React-Framework for the solution so there might be a lot of things you don`t understand yet. But I think the App.css file might help you, especially for media queries

  0