Skip to content
OverviewSolutions
14
Comments
7
Brian Johnson
@BrianJ-27

All comments

  • Brian Johnson•210
    @BrianJ-27

    Kimue, hey!

    So great job in coding this out, the design is pretty much spot on. To answer your question first, remove margin-top, margin-bottom and margin-left styles from your .card-container__img class. Then change the width on that class from 250px to 100%. Next, on your .card-container class, add the shorthand padding property of 10px to create even space around your qr image and that should do it my friend! Let me know if that works for you.

    Also there were a few things I noticed in your code that can remove the accessibility and HTML flags.

    • To remove the 1 accessibility flag on FEM, you need to have a level one heading (a.k.a) an <h1> tag in your project. The way you can fix this is simply replace your <h3> tag with an <h1> tag and reduce your font-size if need be to make it fit the design.

    • Also <article> & <section> tags per web coding standards should have a heading tag nested within them. In your code, you have no heading tags nested within those tags which is why you are getting those 2 HTML issues. To fix that, change them to divs. Like this:

    <section class="card-container"> to <div class="card-container"> 
<article class="card-container__img"> to <div class="card-container__img">
    • Regarding using a background-image to bring in the qr image, it would of been better here to use an <img> tag. Using an image is good for a few reasons. One thing is it has more semantic meaning. Also if you want your images to be indexed by google use an img tag. Background images aren't indexed automatically. But you are not totally wrong to use it here. Just think about when it is appropriate to use them and when it is better to use img tags. Here's a good article on this: Img vs Background Images

    Hopefully this helps and happy coding!

  • Brian Johnson•210
    @BrianJ-27

    Hi @Lou I think you did a good job here but I wanted to show you something. If you open up your inspector tool, you'll notice your content is not going to the bottom of the viewport page. This explains why your card is not perfectly center on the page. How do you fix that? Here's how:

    1. first remove all flex styles from the <body> tag. I know it sounds a little weird, but trust me on this. ;-)
    2. Next you have a <div> tag that has no class associated to it. What I would do here is first change that <div> tag to a <main> tag for accessibility reasons, then cut the class="container" you have on the one div and paste it on the <main> tag. Then rename that class from container to `class="card" because that's what it is. Let me visually show you what I mean.
    <body>
   <main class="container> // changing your div tag to a main tag
       <div class="card"> // rename your class from container to card
           <img src="images/image-qr-code.png" alt="QRCode" class="qrcode">
           <h1>Improve your front-end skills by building projects</h1>
           <p>...content </p>
       </div>
   </main>
</body>

    1. Now on the class="container" add min-height: 100vh. What this does is, makes your container content area go all the way to the bottom of the page. Next you add back those flex properties, display: flex justify-content: center align-items: center . Once you write those flex styles, this will perfectly center your card on the page! you can then remove the margin-top: 100px style bc you won't need it.

    2. another thing you want to think about is using rem units in lieu of pixels. Pixels are fixed units and don't play well with making responsive layouts so try to think about this for future projects.

    3. on your img styles, you don't want to use fixed pixel widths because your image won't scale. On the width I would use max-width: 100% in lieu of width.

    Sorry I wrote a lot of things to change but try to implement these things and see if it helps you :)

    Marked as helpful
  • Brian Johnson•210
    @BrianJ-27

    Hey Peally,

    Good job with getting this project completed. I do have some suggestions that can can help you.

    1. Please take a deep look at your basic HTML structure. It is incorrect. especially your <section> tag that has your <body> tag nested within it. Please remove that section tag from your code. Here's a link that will walk you through how to properly setup a basic HTML boilerplate structure. [https://www.sitepoint.com/a-basic-html5-template/] Refactor your html structure and you will lose a lot of these HTML issues above.

    2. In your code you are not correctly using <section> tags. When you use them, you have to have heading tags nested within them like an <h2> tag for example. So I would simply change your <section> tags to <div> tags.

    3. you must always have one main landmark or <main> tag on the page for accessibility reasons. so in your case, change your <section class="top"> to <main class="top"> and the closing tag too..

    So those are a few things to start but overall good job my friend

    Marked as helpful
  • Brian Johnson•210
    @BrianJ-27

    Hey Kpax

    Overall the layout looks really good visually. You matched the box shadow perfectly on the cards and I may have to steal that color from yours and add it to mine! :) Also great job in not getting any accessibility and html errors. When I checked out your code, there a few things I noticed:

    1. Avoid using fixed widths and try using more flexible widths like % or rems. "ex) width 1100px" The only time I would want to used a fixed width is if it was for something intentional. So for example, I only want my card to be a maximum width of 400px so in this case, instead of using width, I would use, "max-width: 400px". This is better and more flexible because your card's width will adjust to the screen until it reaches 400px and then it won't grow no more. Also any where you are using px's see if you can't convert to rems unless you intentionally need it to be px's.

    2. I see you are using Sass which is awesome and I noticed you are still using the @import directive. Very soon this will be deprecated and be replaced with @use & @forward. If you need some help in implementing it, on Youtube, Kevin Powell provides a great breakdown on how to do this. Here's the link. https://www.youtube.com/watch?v=CR-a8upNjJ0&t=10s

    3. You have unused styles in your style.css. To be more specific, on your .card-container class on lines 119-126 you don't need, flex-direction: row (on line 121) because display: flex will default to a row direction.. Also you don't need "margin: auto & width: 1140px" (on lines 124 & 126) because you already defined "justify-content: center" which is centering your content horizontally, so now we just cleaned up your css a bit. :) So my suggestion would be to take a moment and refactor your code and see if you can make it more cleaner.

    But overall great job

    Marked as helpful
  • Brian Johnson•210
    @BrianJ-27

    Great Job overall.

    I checked out your code and visually it looks good. If you want more specific feedback, I like to try to match the design even down to how many lines of content are there. In your solution, there are 2 lines for your content and the design shows it breaks to a third line. I would maybe increase your font-size a bit or put a little padding on each side of your p tags.

    Also to fix your accessibility issue:

    1. change your div tag to a main tag (or main landmark)
    2. change your h3 to an h1 (each page should have at only 1 h1 tag per page)

    This should clear up your accessibility flags but overall nice job and happy coding!

  • Brian Johnson•210
    @BrianJ-27

    Hey There @dominikapap Great job overall laying out this project. You have this card square in the middle and that is awesome!

    I have a few things to share with you regarding your codebase. I'll just number them for you :)

    1. for your <section class="main"> tag, change the <section> tag to a <main> tag

    2. for your <section class="footer"> tag change the <section> tag to a <footer> tag **Formatting your code this way should remove all of your accessibility & HTML issues with the code

    Regarding best practices, I see you have 2 css stylesheets in the root directory of your project. It is usually best to create a "css folder" then place those 2 stylesheets within that folder. When you do that, you next need to go back to your index.html file and modify your filepath to the stylesheets.

    That's all I see for now at quick glance but great job overall. Happy Coding

    Marked as helpful
  • Brian Johnson•210
    @BrianJ-27

    Hi Naveen! Thanks for your feedback. So yes I already added the main tag but what I didn't add when I first published this project was the h1 tag (which I believe caused the accessibility issue). I went back and updated the code to include that but it didn't refresh and take away the accessibility flag.