Skip to content
Submitted 10 months ago

3-Column-Preview-Card

m07mmad-nasr•160
@m07mmad-nasr
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...3-column preview card component
  • HTML
  • CSS
1newbie

Design comparison

SolutionDesign

Solution retrospective

Evaluate My Work

Community feedback

  • Øystein Håberg•10,710
    @Islandstone89Posted 10 months ago

    HTML:

    • For clarity, I would give <main> a class - something like card-container, for example.

    • The icons are decorative, meaning the alt text should be empty: alt="".

    • All the headings are on the same level, so it makes sense for them to be the same element. Since there can only be one <h1> on a page, make all 3 of them into a <h2>.

    • "Learn More" would navigate to another page, hence it is not a button but a link.

    CSS:

    • Performance-wise, it's better to link fonts in the <head> of the HTML than using @import.

    • It's good practice to include a CSS Reset at the top.

    • Remove all positioning and transform properties on main.

    • You have a typo on main, which says: width: 1005;. I assume you meant width: 100%? That is not needed - main is a block element, meaning it takes up 100% of its parent width by default.

    • To center the card container horizontally and vertically, use Flexbox on the body:

    display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
min-height: 100svh;

    • Remove all widths and heights in px.

    • Add some padding on the buttons: 1em 3em worked OK in my DevTools.

    • Give the card container a max-width in rem, so it doesn't stretch too wide on big screens.

    • Paragraphs have a default value of font-weight: 400, so there is no need to declare it.

    • The headings need to be in ALL CAPS. To do this, use text-transform: uppercase;.

    • Media queries should be in rem, not px. Also, it is common practice to do mobile styles first and use media queries for larger screens. So, the card container should have the following grid properties as default:

    display: grid;
grid-template-columns: 1fr;

    Which on larger screens would switch to: grid-template-columns: 1fr 1fr 1fr;

    or you could use a "shortcut": grid-template-columns: repeat(3, 1fr);

    1
  • solvman•1,650
    @solvmanPosted 10 months ago

    Hi @m07mmad-nasr,

    Very well done! 🥳🎉🎊

    Your project looks very clean, and I'd like to complement you on a few things:

    📌 Nice CSS reset and use of CSS global variables for colors and rem for font sizes.

    📌 I like your use of <article> for each card. 👍

    📌 Very nice use of <h1> through <h3> 👍

    📌 HTML is very clean! Proper use of semantic HTML. There are no unnecessary wrapper <div>s

    I have a few suggestions for improvement:

    📌 It looks like the middle paragraph might use a bit larger line height, which could be achieved with:

    article p {
    line-height: 1.75; /* play with it and see what looks the closest */
}

    📌 If you are trying to achieve a truly responsive design, do not use fixed size, height, and width. Look into employing a combination of min-height, max-height, min-width, and max-width. Check out how I utilize those for the same project here

    Otherwise, very well done! 🚀  Congratulations! 🎊🎉🥳

    0

Please log in to post a comment

Log in with GitHub

Join 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