Skip to content
OverviewSolutions
12
Comments
19
Lidia Krajewska
@lidiakrajewska

All comments

  • Lidia Krajewska•390
    @lidiakrajewskaPosted almost 2 years ago

    Hi! Great desktop view! I've got a few suggestions to make mobile and tablet view better.

    • I'd change the media querry 375px to something around 500px, as it covers various mobile screens sizes that are in use (now the layout breaks on e.g iPhone XR which is 414px)
    • Instead of setting body's height, it's a good practice to set min-height (that way your wrapper element has the room to grow and the content won'y overflow on smaller devices). The same applies to .wrapper height.
    • It would look good if you'd add also margin-bottom to .accordian-wrapper
    Marked as helpful
    0
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Hello! Great job on the card's look :) I've got a few tips on how to make it more responsive. Right now if you change the font size in your browser to bigger, the card breaks (there's an overflow). It's caused by giving the card a fixed height. It's recommended to use min-height instead. But because you use absolute positioning the card will stretch then, so I'd suggest to use Flexbox to position the card instead.

    body {
display: flex;
min-height: 100vh;
}
    0
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Good job!

    I noticed that on mobile the component is not centered perfectly. It's caused by the attribution div. You can take attribution div outside of the container and add to body styles:

    body {
  flex-direction: column;
  align-items: center;
}

    This should solve the issue. Also you can wrap your container div in <main> tag (it'll solve the accessibility issue) :)

    Marked as helpful
    0
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Great job, that's a very elegant solution!

    To solve the accessibility issues you can wrap your card div in <main> tag and change your h2 to h1 and h3 to h2 :)

    Marked as helpful
    0
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Hi! You did a great job, it looks very similar and the code seems logical.

    But I've got a suggestion about media queries. As a general rule it's good to have as little media queries as possible, because it makes the code easier to maintain. Ussually no more than 3 media queries is needed. Actually you can build this one without any! :) You can try out this code:

    .wrap {
  width: min(87%, 400px);
}

    On small screens it will has the width of 87%, but once that 87% will get bigger than 400px it won't grow anymore. That way your component will look great even on big screens. (Now it stretches on 2K monitor).

    Marked as helpful
    2
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Hello! Good job!

    I've got one suggestion - as best practice try to avoid fixed heights. Try using min-height / max-height instead, it will allow each component to resize if needed and will prevent overflow, so overall will make your web pages more responsive.

    0
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Very elegant solution, loved how you organised your HTML and CSS!

    1
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Hello! You did a great job on this one :)

    One thing I noticed is that on mobile view the footer can overlap with the card component, to fix it just change the height property of the container in media querry to min-height. You can also try out CSS Grid in projects with similar layout, as it's great for layouts that are two dimensional and need to change on smaller devices.

    Marked as helpful
    1
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Hi! I've got a few tips that should make creating layouts easier for you :) Instead of using position property to center your component try using flexbox, it's super useful and makes it way faster once you understand it. The height of your component will be based on the content, so there's no need to specify that. To position elements inside of it, try using padding and margins instead of position. Also, by default, images, headings and paragraphs are displayed each in a new line, one under another, so unless we specify it otherwise in our CSS, the structure will be correct. I hope this will help you somehow, happy coding :)

    Marked as helpful
    0
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Hello! As the name of this challege suggests, it's best to use grid in this project. It will make your page responsive and it will be way easier to position each part of this component. Kevin Powell on YouTube has some great introductory videos about CSS Grid if you'd like to try using it :)

    Marked as helpful
    0
  • Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Hi! It would be good if you wrap all your content in main tag, except for attribution that could be a footer. Also don't leave empty h1 tag, instead you could have " Improve your front-end skills by building projects" as your h1, because we use h1-h6 headings to imply hierarchy, not because of their font size (it should be adjusted with font-size property). A thing that one has to take into consideration these days is also how your page looks on mobile devices - if it's responsive. To check it use DevTools in your browser and click "Toggle device toolbar", you can choose what kind of device should your browser simulate. Happy coding and good luck!

    1
  • Yous•170
    @yousra10Submitted over 2 years ago

    Hi everyone!!!

    I just completed that challenge, I find it easier than the others. Please, if there is a point that I have to change, tell me. Thank you in advance.

    Happy coding!!

    qr-code-component-main2

    1
    Lidia Krajewska•390
    @lidiakrajewskaPosted over 2 years ago

    Hi! The image doesn't show up, you need to change the file path to ./image-qr-code.png :) If you'd like to center the component both vertically and horizontally the easiest way is to use flexbox. You could wrap container div in another div or just add this code to main:

        display: flex;
    align-items: center;
    justify-content: center;
    min-height: 100vh;

    And then you need to remove margins from container. Happy coding! :)

    Marked as helpful
    0