Mohammed Abbas
@Mohammedabbas7All comments
- @AbuSalman143Submitted 9 months ago@Mohammedabbas7Posted 9 months ago
Hello
Congrats on completing this challengeππππ.
Nice solution and also like what you did with the attribution.
I noticed a few things in your solution you need to adjust:
- First instead of adding
height: 100vh
to the body element because this causes some overflow issue so addmin-height: 100vh
to make the responsive. - Second instead of adding
height: 60%
to the .content element just add gap propertygap: 1rem
. - you did add a height property to .container element that is not good practice, so don't add specific height to elements unless it's so important to add it.
I hope my advice is useful.
Happy Coding :)
0 - First instead of adding
- @ZbidaMohcineSubmitted 9 months ago@Mohammedabbas7Posted 9 months ago
Hello
congrats on completing this challenge ππππ.
nice solution by the way but I have a few adjustments for you:
- instead of using the
flex-wrap: warp
property on the .root element to make the layout for the mobile version to make the site responsive for different screen sizes you can use media queries like this:
@media(max-width: 768px) { .root { flex-direction: column; } }
- also I noticed that you did not add the hover effect to the button but you did add the focus effect, you can add this code to add hover:
button:focus, button:hover { background-color: transparent; color: white; }
- also you can add the transition properly on the button to make the transition smooth use this property
transition: all .3s;
.
I hope you find these adjustments useful.
Happy Coding :)
Marked as helpful0 - instead of using the
- @matthewadeboyejrSubmitted 10 months ago
Your review will be appreciated
@Mohammedabbas7Posted 10 months agoCongrats on completing this challenge ππππ
Nice solution.
I have some advice for you
- the hover effects on the img-overlay instead of using
display: none;
it's better to useopacity: 0;
on the img-overly and also instead of usingdisplay: block;
on the main_img-box:hover .img-overlay rule you can useopacity: 1;
- also you can add transition property on the img-overly to make the background-color
and the icon have some transition when you hover over it, add this
transtion: opacity 0.5s;
.
That is all I have, I hope you find it useful.
Happy Coding :)
Marked as helpful1 - the hover effects on the img-overlay instead of using
- @shank-codesSubmitted over 1 year ago
- Is it ok to use absolute positioning css property when it serves the purpose.
@Mohammedabbas7Posted over 1 year agoCongratulation on completing this challenge, well done πππ.
First, regarding your question, it's okay to use position absolute property when it needs it but in this scenario, you did not need it.
Second, I guessed that you used position absolute to center the card in the middle of the screen this better way to center the div is by using flexbox, here is:
HTML
- create a div with a class of container inside the body element.
- inside that container create a div with the class of card to hold the content of the card.
<body> <div class="container"> <div class="card"> <div> <div> </body>
CSS
- add this style to the container to center the card.
.container { min-height: 100vh; display: flex; align-items: center; justify-content: center; }
You can test this code and tell me if it works for you instead of using the position absolute property.
If you don't know about flexbox watch this video by Kevin Powell.
Happy coding :)
Marked as helpful1 - @DavidCaffreySubmitted over 1 year ago
Looks okay to my eye. What do you think?
@Mohammedabbas7Posted over 1 year agoCongratulation on completing this challenge, well done πππ. I have a few suggestions for you:
- instead of using the main element to center the card you could create div with class of container inside the main to center the card, and inside the container create another div with the class of card to hold the column like this:
HTML
<main> <div class="container"> <div class="card"> </div> </div> </main>
CSS
.container { display: flex; align-item: center; justify-content: center; min-height: 100vh; }
- I noticed that you gave the .car-type class height property. it's better to let the content define the height of its parent instead of give in it a specific height.
I hope what I gave you is helpful.
Happy coding :)
0 - @simonmatt89Submitted almost 2 years ago
This is my 2nd project that I have completed with Front End Mentor. This time, I hope I have improved my use of sementic HTML code. I do wonder if my CSS code could be more efficient? I'm looking forward to checking out other's solutions so I can compare my code.
I also find it easier to create a mobile site than I did previously. I was able to create this project much faster than the last which makes me happy!
@Mohammedabbas7Posted almost 2 years agoCongratulation on completing the second project π. I have a few adjustments that I wanna show it to you.
- replace the <div> tag with <footer> tag on the attribution section because it has semantic meaning.
- you forgot the hover state on the Learn More buttons. you can add these properties to the button element
button { display: inline-block; background-color: "color that you choose"; color: #333; padding: .8rem 1.5rem; border: 1px solid "color that you choose"; border-radius: 2rem; transition: all .2s; }
for the hover state you can add these properties:
button:hover { background-color: transparent; color: "color that you choose"; }
well done on this challenge and happy coding on the next challenge π.
Marked as helpful0 - @HatchinoSubmitted about 2 years ago
I'm currently stuck on a challenge, in the meantime I tried this one which is simpler. Feel free to leave any suggustions !
@Mohammedabbas7Posted about 2 years agohello, nice solution. I just wondering why you used internal CSS instead of external CSS.
0 - @ibrahimtangcoSubmitted about 2 years ago
Hello everyone! Here I made changes to my solution to this challenge. I followed your suggestions and comments on my previous solution. Thank you!
@Mohammedabbas7Posted about 2 years agocongratulation on completing the challenge. I just have a single comment on your solution you just forget to add a hover and focus state on the button. you can add this code to change the background color when you hover with the mouse or use the keyboard to navigate and also you can add a transition property on the button to make the transition smooth.
button { transition: background-color .5s; } button: hover, button: focus { cursor: pointer; background-color: hsl(159, 63%, 16%); }
0 - @markosbahgat111Submitted over 2 years ago@Mohammedabbas7Posted over 2 years ago
Hello, congratulation on completing the challenge.
- you just need add hover effect to the button by add this sytle to your section button rule, and remove border: none;
section button { border: 1px solid #fff; } section button:hover { background-color: transparent; color: #fff; }
1 - @maquindeSubmitted over 2 years ago
Hello All,
Please review my solution! Any feedback is welcome :)
@Mohammedabbas7Posted over 2 years agoHello, nice work by the way. just you have a little problem with your background-image URL, you typed the URL wrong.
you need to be like this
.card-image { background-color: url(../images/illustration-hero.svg; }
.Keep up the good work :)
Marked as helpful1 - @McMuffin1022Submitted over 2 years ago
Hi, I make it in pure html /css. If you have any advice, im interested to know it!
Thanks for your Time, McMuffin1022
@Mohammedabbas7Posted over 2 years agoCongratulations on completing the challenge.
This is just a little advice:
- you just need to add hover effect to the image, title and creation name.
If you don't know how to make it just tell me.
Happy coding:)
0 - @PenaJ15Submitted over 2 years ago
I really liked this challenge
@Mohammedabbas7Posted over 2 years agoCongratulations for completing the challenge.
I just have a little feedback for you, you need to add
.card-image div { transition: opacity .3s; }
for the hover state to work.Happy coding :)
0