Funsho Ayobanjo
@ayobanjoAll comments
- @Yusif70Submitted about 2 years ago@ayobanjoPosted about 2 years ago
Hello @Yusif70, you did well on this challenge
I just have a few suggestions.
To make the main responsive, on all screen sizes, change the
width: 300px
tomax-width: 300px
Also on the body, set the
justify-content: centre
andmin-height: 100vh
, with this, you wouldn't need to set the hard-coded margin-topI hope this helps.
1 - @kagiso101Submitted about 2 years ago@ayobanjoPosted about 2 years ago
Hi @kagiso101 Good Job on this.
Just a few corrections to make it more responsive and less rigid. Mark as helpful if you like it
HTML
Change the div to the more semantic main tag
CSS
-
Set the main div to a
max-width
instead ofwidth
to make it responsive -
Set the img to
max-width: 100%
, to always make sure it retains the width of the container -
Remove the width and height on the h1, and p tags. They are naturally responsive. Then add a
padding
to the main div
Marked as helpful0 -
- @IsuruAkalankaSubmitted about 2 years ago@ayobanjoPosted about 2 years ago
Hi @IsuruAkalanka, Good job on this. Just a few corrections to make it more responsive. Mark as helpful if you like it
HTML
Make the div a main for a more semantic structure.
CSS
-
On the div change the
width
tomax-width: 300px
for responsiveness -
Set the img element to
max-width: 100%
to always keep it within its container
Marked as helpful1 -
- @IndraSaputraIdrusSubmitted about 2 years ago@ayobanjoPosted about 2 years ago
Hi @IndraSaputraIdrus Good job on your first project, lovely
The only observation I have is that for mobile responsiveness, you should use a max-width instead of width on the card div
max-width : 260px
or its equivalent in rem0 - @villAshSubmitted about 2 years ago@ayobanjoPosted about 2 years ago
Hello @villAsh. Congratulations on your first submission.
I do have a few corrections you could apply
HTML
-
For SEO, and proper semantics, you should use the
<picture>
tag. This will help you to also change from either mobile img to desktop img depending on what your approach is (mobile-first or not) picture tag -
The Gabrielle Essence Eau De Parfum should be an
h1
element, and the perfume a paragraph -
For the former price, you can simply use the
del
tag
CSS
-
The height of the
img
should be 100%, so it simply fills the available space in its container -
The perfume has a letter spacing, which you have omitted, try
letter-spacing: 0.25rem
, or whichever value you find desirable -
You can set a
max-width
for your main container on the desktop view.
Here is my own solution. I hope you find this helpful my submission
Marked as helpful0 -
- @KingJosephISubmitted about 2 years ago@ayobanjoPosted about 2 years ago
Hello @josephmadras, good Job, I do have a few corrections.
For the perfume, there is letter spacing, which you have omitted
letter-spacing: 0.25rem
or whatever spacing you find desirable.....Hope this helps
Marked as helpful0 - @Deepanshu-5288Submitted about 2 years ago@ayobanjoPosted about 2 years ago
Your solution is elegant, I like it. Just completed this same task myself. The only thing is the margins added/ gap in the mobile view, use gap or margins instead on space-evenly
0 - @aljaclySubmitted about 2 years ago
This is my first time creating something with HTML and CSS. Any advice on best practices is appreciated.
I struggle with understanding how to use layouts more effectively, hence why "px" is used a lot when positioning elements.
I am also unsure if I am using CSS selectors correctly with the div tag.
@ayobanjoPosted about 2 years agoHello Aljacly, I just submitted this myself, few observations though #CSS
- You should create a CSS file to make your HTML file cleaner
- You should use flex on your body tag to centre its children(main or the div containing the code)
- The div class border is breaking the code. You should remove it once the flex is added
#HTML
- Your text should be in a paragraph tag, or heading tag, not in a div
Hopefully, I have been of help. Still a lot to be done though
Marked as helpful1