Skip to content
OverviewSolutions
24
Comments
30
Martin
@ringm

All comments

  • Martin•1,150
    @ringmPosted over 3 years ago

    Hey! Amazing job, the site looks exactly like the design. Congatulations!

    Regarding your question: if you define your grid to be 1 column wide, all the child elements of the grid will fit like you want.

    .container {
  display: grid;
  grid-template-columns: 1fr;
}

    Keep up the good work!

    0
  • Mandeep Sharma•10
    @mandeephubSubmitted about 4 years ago

    pls provide a detailed feedback Im coding for this platform for first time so pls be specific about the bugs thanks in advance

    Social proof section

    1
    Martin•1,150
    @ringmPosted about 4 years ago

    Looks good! Congratulations on completing your fist project.

    My only suggestions would be vertically centering the content for tablet/desktop devices.

    Looking forward on seeing your next projects!

    0
  • Martin•1,150
    @ringmPosted about 4 years ago

    Looks good! Just remember for accesibility reasons, you should have a <label> for every <input>.

    Your form should be structured like this:

    <form action="/action_page.php">
  <label for="firstName">First Name</label>
  <input type="text" name="firstName" id="firstName" value="First Name">
</form>

    More info on labels here

    Happy coding!

    1
  • Martin•1,150
    @ringmPosted about 4 years ago

    Hey Nazar, congratulations on submitting your first challenge. It's looking good! I have some suggestions to help you improve it:

    • instead of using fixed margins on the .content-wrapper, consider using something like width: min(90%, 1100px); and margin: 0 auto;. With these two you'll get a fluid and centered container.
    • for the header, remove the flex: wrap;. Instead, try flex-direction: row; and when it gets too narrrow you change it to flex-direction: column;
    • same as the header, remove the flex-wrap from .gallery-wrapper and also remove the fixed width on it's child elements, let them be fluid and stretch to the available space. once the screen get's too narrow, you can change the flex-direction on .gallery-wrapper to column.
    • also, the HTML report is yelling at you because you didn't include any h2-h6 tags inside the <section>. To avoid this problem, you can either change the sections elements for regular divs or include headings and hiding them.

    Keep at it! I'm looking forward to your next project!

    1
  • Martin•1,150
    @ringmPosted about 4 years ago

    Hey Shubham, the site looks good! Couple suggestions to polish it:

    • Consider setting the background image in css through the background property. There you can also set the background-size and background-position to place it exactly where you need.
    • For a responsive layour, try the following:
    .wrapper {
  display: flex;
  height: 100vh;
  width: 100vw;  
}

.bg-image {
  background-image: url(../images/bg-mobile.svg);
  background-color: #0c122c;
  background-repeat: no-repeat;
  background-size: cover;
}

.container {
  width: min(90%, 860px);
  margin: auto;
}

.row {
  display: flex;
  flex-direction: column;
}

.col-30,
.col-70 {
  width: 100%;
  border: 1px solid red;
}

@media (min-width: 600px) {
  .row {
    flex-direction: row;
  }
  
  .col-30 {
    width: 30%;
  }
  
  .col-70 {
    width: 70%;
  }
}

    <div class="wrapper bg-image">
  <div class="container">
    <div class="row">
      <div class="col-30">Fylo block with logo and buttons</div>
      <div class="col-70">Fylo block with the bar and storage info</div>
    </div>
  </div>
</div>

    The class bg-image should be used to place the background image.

    That should take care of the main layout for mobile and desktop, I hope it helps! Let me know if you have any doubts with the code.

    1
  • Martin•1,150
    @ringmPosted about 4 years ago

    Looks amazing! Exactly like the design! Congratulations :)

    1
  • Allison Bates•170
    @allisonkbatesSubmitted about 4 years ago

    Any suggestions on how to reduce the CSS needed for the change in layout between mobile and desktop?

    Basic HTML & CSS

    1
    Martin•1,150
    @ringmPosted about 4 years ago

    Hey Allison! Great job, the site looks really good! Well done.

    Regarding your question, I would suggest trying a layout with flexbox like this:

    .container {
  width: min(90%, 1100px);
  margin: 0 auto;
}

.row {
  display: flex;
  flex-direction: column;
}

.col {
  width: 100%;
}

@media (min-width: 600px) {
  .row {
    flex-direction: row;
  }
}

    <div class="container">
  <div class="row">
    <div class="col">H1 and text</div>
    <div class="col">Ratings</div>
  </div>
  <div class="row">
    <div class="col">Comment 01</div>
    <div class="col">Comment 02</div>
    <div class="col">Comment 03</div>
  </div>
</div>

    This would take care of the overall layout both for mobile and desktop, I hope it helps!

    Happy coding!

    1
  • Martin•1,150
    @ringmPosted about 4 years ago

    Hey Alex! Great job! The site looks great. I have tiny suggestions to polish it.

    • Center the main container so the content sits nicely on the middle of the screen (in your case, maybe adding margin: 0 auto; on the <main>. Although as a better practice, I would suggest wrapping everything in a <div> and give it a container class.
    • Your <input> should have <label>, that's why you're having html issues on your report. Also, consider wrapping your forms with a <form>, it's a better practice from the semantic point of view

    An example would be:

    
<form>
  <label for="firstName">First name:</label>
  <input type="text" id="firstName" name="firstName" value="John">
  <label for="lastName">Last name:</label>
  <input type="text" id="lastName" name="lastName" value="Doe">
</form>

    Just tiny things! Overall it's a great job! Keep at it!

    2
  • Martin•1,150
    @ringmPosted about 4 years ago

    good job! overall it working pretty well. I'm also using tailwind for my latest submissions and I feel it really improved my workflow.

    Tiny suggestions:

    • You're missing the alt attribute on the img tags, which doesn't create any problems in the visual side, but for accesibility purposes you should always include it.
    • I feel the background image would look much better if it would cover the entire size of the viewport. On mobile and tablet looks fine, but on desktop looks weird. Also consider giving a max-width to the container so it doens't stretch infinetly ;)

    Awesome work! Looking forward on what you build next!

    0
  • Martin•1,150
    @ringmPosted about 4 years ago

    looks amazing! really neat animations and the responsiveness is sweet.

    tiny suggestion: maybe adding a max-width to the container? I'm opening it on a 4k monitor and, although it's not breaking (which is really nice) I feel is stretching too much, and the font display at that size is too small.

    Other than that, it's great! Keep at it!

    1
  • Martin•1,150
    @ringmPosted about 4 years ago

    Hi Onyeanuna! Great job completing this challenge, it’s looking really good!

    However, there’s an issue with the main img, on small/medium devices it’s ok, but on large devices it grows extremely big and brakes the design! A fast duck-tape solution could be setting a max-width so it doesn’t stretch indefinitely. Another solution would be to make some layout adjustments to the whole layout for bigger screens with media queries.

    Keep coding!

    1
  • Martin•1,150
    @ringmPosted about 4 years ago

    Looking great! I think the only thing missing is box-shadow on the main component.

    Here's a box-shadow generator that can help you achieve exactly the look you want with ease: https://www.cssmatic.com/box-shadow

    Great work!

    0