Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

3 columns preview card

JJ 160

@JeremyPaymal

Desktop design screenshot for the 3-column preview card component coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Solution retrospective


Please tell me if I can improve this code.

Thks !

JJ

Community feedback

Ahmed Hany 220

@ahmedhanyh

Posted

Hey JJ! Amazing work!

There are some things you can do to improve your solution:

  • You can remove the unnecessary border-style: none; declaration in your css reset as the initial value for the border-style property is already none.
  • The <main> element doesn't need to be a flex container in this case as it contains only a single child element, the display: flex; declaration can be removed. To center the <main> element in the page, give the body a min-height: 100vh and use either flexbox or grid to center it both vertically and horizontally, like this:
/* Using flexbox */
body  {
  min-height: 100vh;
  display: flex;
  justify-content: center;
  align-items: center;
}
/* Using grid */
body {
  min-height: 100vh;
  display: grid;
  place-content: center;   /* or place-items: center; */
}

and remove the margins around the <main> element.

  • Try to use a layout method like flexbox or grid to layout/space the content inside .main_content-wrapper elements instead of using margins.
  • Set the border-radius on the .main_content element with overflow: hidden; instead of specifiying each element corner's radius, like so:
.main_content {
  border-radius: 0.5rem;
  overflow: hidden;
}
  • To implement the active states for the 'Learn More' buttons, give them a border, change their background-color to transparent and their text color when hovered over using the :hover pseudo-class as in this code snippet:
a {
  ...
  border: 2px solid var(--color-bg-hd-btn);
}

a:hover {
  background-color: transparent  /* or initial */
  color: var(--color-bg-hd-btn);
}

That's all I have. I hope you found the review helpful and I wish you the best with your future challenges.

Marked as helpful

0

JJ 160

@JeremyPaymal

Posted

@ahmedhanyh Hello !

I did the modifications and it works perfect !

Thanks for all the advices ! It was very helpful.

JJ

1
Ahmed Hany 220

@ahmedhanyh

Posted

@JeremyPaymal You're welcome! Glad it worked!

0
Adriano 33,970

@AdrianoEscarabote

Posted

Hello JJ, how are you? I truly loved your project's outcome, however I have some advice that I hope you'll find useful:

To align some content in the center of the screen, always prefer to use display: flex; it will make the layout more responsive!

Example:

body {
    margin: 0;
    padding: 0;
    display: flex;
    align-items: center;
    flex-direction: column;
    justify-content: center;
    min-height: 100vh;
}

I noticed that you used 2 h1 tags, this is not recommended as we can only have one h1 tag per page to inform the main title, prefer to use h2 since this challenge is only one component!

The remainder is excellent.

I hope it's useful. 👍

Marked as helpful

0

Please log in to post a comment

Log in with GitHub
Discord logo

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