Parfum Card

Community feedback

• 

  Lucas 👾•104,400

  @correlucasPosted over 2 years ago

  Hello OLEKSII, congratulations for your solution!

  I took a look on your solution and I have some tips for you:

  1. Remove align-items, justify-items and the margins from the container instead of that, apply flex do the body tag, like the following code:

  body { height: 100vh; display: flex; font-family: 'Montserrat', sans-serif; align-items: center; justify-content: center;}

  2. When you apply height: 100vh, this will make the page body display 100 of the viewport height, using the flex and the alignment on body this will center all content inside the tag.

  3. You can also apply a proper color for the card, thats white and a different color for the background.

  4. Accessibility issues: you can check the report on your solution panel, by what I look you need <main> tag to wrap your content and at least one h1 heading.

  I hope it helps you!

  Marked as helpful

  1
• 

  Luiza Uszacki•250

  @LuizaUszackiPosted over 2 years ago

  Hi @joopWU.

  I saw your body doesn't have a height. Therefore, its height is the same as the container. So, to make it have all the screen's height as the height you might want to add min-height: 100vh. With that, your body will have at least all the screen's height as its height. After that, you can add the background-color.

  Doing that, your screen will seem a little bigger than the screen's height, tho. It's because, by default, some elements have a margin, and your body has a default margin of 8px on each side. So it's the screen's height plus 16px (8px in the top and 8px in the bottom). You could reset this with body { margin: 0;} or you could apply it to all elements using *, it's up to you.

  I tried doing that in your project with dev tools and it created a new issue. Your container will be stuck in the body's top and will add a margin to the top of the body instead of between the body and the container. If you want to know more about it you can click on this link Everything You Need To Know About CSS Margins. The "PARENT AND FIRST OR LAST CHILD ELEMENT" part shows exactly what will happen to your project and how to solve it.

  So... If you add something like border: 1px solid transparent; it might solve it. But it will make the body's height bigger than the screen's size again. It's because the height will be 100vh + 2px (1px each side). To avoid that you can set box-sizing: border-box to all your elements (*) and they will have the size you set without adding anything more because of the padding or border.

  But if you don't want to run into this issue, what I highly recommend is using flexbox to center your card. (still resetting the element's margin, setting box-sizing: border-box and having the body's min-height as 100vh).

  0
• 

  Stefani Josifovska•270

  @stefani-josifovskaPosted over 2 years ago

  Hi! I just opened your solution and first of all, I would like to point out that I love the structure, it is really clean. I only have one possible improvement, which is basically a solution for your desktop-, as well as mobile version at the same time, and it has to do with the positioning of the "container" element. Basically, you can also see on the segment on this page as well that your container is not perfectly centered vertically. I noticed that you have tried to do this by setting margins on the "container" div, which would normally work, but doesn't because you have also set margin-top: 10vh, so it's overwritten. Anyways, there are a few possible solutions, but the one that I have tried on your webpage and works perfectly is to work with the css of the "body" element. Namely, you have to first get rid of the automatic CSS that body has, by specifying margin: 0, and then you can set the body-height or minimum body height to 100vh (something in the spirit of your margin-top: 10vh approach), and then turn the display property of the body element to flex -> and you know what to do afterwards in order to center the div. :) I wouldn't say this is the best or the only approach, for that matter, but it's just one approach worth trying. *This would center the "container" div perfectly both in mobile and desktop version. However, you should maybe add top and bottom margins on the "container" div in mobile version, in order to make it look nicer. And maybe also add a background color to the body, to make it look nicer, as in the original design. Other than that, the solution is looking really nice! **And just another note regarding the css code: if you don't need to style a given element, there's no need to give it a class name and then defining an empty definition for that element. Just don't assign anything to it, in order to prevent redundant and unnecessary code. I hope I helped even a little bit. Good luck coding!

  0