Design comparison
Solution retrospective
What's your opinion ?
Community feedback
- @antaryaPosted almost 3 years ago
Hi 👋,
Great job, it looks really good.
I have a couple of suggestions/improvements, though:
HTML
[1. Wrap your content with the
main
tag. That way browser will know where to find primary content. Also, it will fix some accessibility issues. Here is a resource with examples related to semantic tags.[2. Add
alt
attribute toimg
tag. It can be empty or should describe what is on the image. Screen reader users should have an idea of what is on the image based on the alt text. Here are some resources: mdn alt, great-alt-text.[3. HTML and CSS files are not formatted properly. It will make your code more readable if you use an editor extension that can format on each save for you. For example, if you are using vscode try prettier, for different editor search for
editor-name prettier
.CSS
[4. Consider more universal approach for
box-sizing
using this method.[5. Take a look at this thread regarding different ways to name CSS colour variables, so it is easier to use while styling your app. slack discussion.
[6. You are using
:nth-of-type(N)
pseudo-class a lot, which looks not very efficient. Next time you want to insert another card in the middle, you will have to change the rest; instead, you can create.card--bg-pink
or.bg-pink
and apply where necessary. Another example would be to have modifier.card--with-quote-bg
, which will add quote background. That way, you make your code easier to read; and easier to apply changes. Here is a resource on how to structure your CSS using [BEM methodology] (http://getbem.com/introduction/), also later, you might be interested in this scss and this tailwindcss.[7. Where possible, try not to use fixed width and height so your elements can adapt to different screen sizes, limit using max-, min- version of width and height where required but allow to adapt. Also, it feels like not the best choice to use
em
, unless that was the intention to bond width to font size. One of the possible solutions usinggrid
but without fixed width/height would be to remove all width/height except on containers and then have something like this:/* For this to work first in HTML, move current card#5 to be card#3 so it is ordered the way they appear. */ .card { height: 100%; } @media(min-width: 1200px) { .container { padding: 20px; } .grid-wrapper { display: grid; grid-template-columns: repeat(4, 1fr); grid-gap: 20px; } .card:nth-of-type(1) { grid-column: 1 / 3; grid-row: 1; } .card:nth-of-type(2) { grid-column: 3; grid-row: 1; } .card:nth-of-type(3) { grid-column: 4; grid-row: 1 / 3; } .card:nth-of-type(4) { grid-column: 1; grid-row: 2; } .card:nth-of-type(5) { grid-column: 2 / 4; grid-row: 2; } }
There are other ways to achieve this, e.g. using grid-template-areas.
[8. It would look better if you move
background-color
to thebody
tag, so all area is filled with background.[9. Avatar size is not equal as it has
width: 12%
, so it depends on the size of the container. In this case, you might benefit from using fixed sizes as you know it should have the same size on all cards..img-div { width: 45px; height: 45px; }
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Marked as helpful0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord