Skip to content
OverviewSolutions
4
Comments
4

Alexej Kunz

@Orchi1904Hof, Germany60 points

I recently finished studying computer science at Hof University. During my studies, I noticed that I particularly enjoy web development.

I’m currently learning...

⚛️ React/Next.js 💨 Tailwind

Latest solutions

Latest comments

  • Alexej Kunz•60
    @Orchi1904

    Hey @aaishver,

    I am very happy with your solution, it looks great. There are only three small things that make it better in my opinion:

    • It is great that you used a footer element for the attribution but the footer element should be inside the body element, because the footer is part of the website

    • Your solution looks even better if you center the card. This is possible if you add the following things to your body-selector in your CSS:

    body{
    padding-top: 8rem;
    background-color: hsl(30, 38%, 92%);
    margin: 0; /*Get rid of unnecessary margins*/
    padding: 0; /*Get rid of unnecessary padding*/
    height: 100vh; /*Body height is now full viewport height*/
    display: flex; 
    flex-direction: column; /*Use this so that the card and the footer are below each other and not next to each other*/
    justify-content: center; /*Center the card vertically*/
    align-items: center; /*Center the card horizontally*/
}
    • <p> are not allowed as a child elements of <button>. You should rather use a <span> element inside the button like so: 
    <button>
   <img src="images/icon-cart.svg" alt=""/>
   <span>Add to Cart</span>
</button>

    I hope this was helpful for you. Happy coding!

  • Alexej Kunz•60
    @Orchi1904

    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!

  • Alexej Kunz•60
    @Orchi1904

    Another thing I see is that my solution that is displayed at the design comparison is not centered at all while it is centered if you click on the preview site. I guess it might be some error here on frontendmentor, because I also could not find any solution so far that is centered in the design comparison.

  • Alexej Kunz•60
    @Orchi1904

    Hey MarkyDDev,

    I looked through your solution and really like it but there is one easy fix that makes it much better. You could center your container not only horizontally but also vertically. This can be achieved by changing your body-style in CSS from this:

    body {
  box-sizing: border-box;
  font-family: 'Poppins', sans-serif;
  margin: 1.5rem;
  background-color: hsl(212, 45%, 89%);
}

    to this:

    body {
  box-sizing: border-box;
  font-family: 'Poppins', sans-serif;
  margin: 1.5rem;
  background-color: hsl(212, 45%, 89%);
  height: 100vh;
  display: flex;
  justify-content: center;
  align-items: center;
}

    As you can see I added four lines of code:

    • height: 100vh; is used so that the height of the body is the full viewport height. This is important for the flexbox, because if you dont specify that, the container will not be centered.
    • display: flex; justify-content: center and align-items: center; are used to center the container both horizontally and vertically inside the body. Justify-content centers it horizontally and align-items centers it vertically in this case.

    With this four lines of CSS you can center the container and your solution looks closer to the original design. You could also reduce the size of the QR-Code a little bit, this would also help to get your solution closer to the original.

    I hope this was helpful to you. Happy coding!

    Marked as helpful