@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 theborder-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, thedisplay: flex;
declaration can be removed. To center the<main>
element in the page, give the body amin-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 withoverflow: 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
@JeremyPaymal
Posted
@ahmedhanyh Hello !
I did the modifications and it works perfect !
Thanks for all the advices ! It was very helpful.
JJ
@ahmedhanyh
Posted
@JeremyPaymal You're welcome! Glad it worked!