@AnaLuisaFav
Pipow
@PipouwPieuwAll comments
- @PipouwPieuw
You did well, I think it looks good on desktop and mobile!
That being said, there's an horizontal scroll on desktop. The section linha-5-imgs seems to be at fault.
In the footer, I guess social media icons need to be linked to their respective social media page.
Your interactive elements (links and buttons mostly) would look even better if you put hover effects on them, like you did on the Contact button. You may also set a transition so the effect would be more pleasing to the eye :)
Well done overall!
Marked as helpful - @leonpahole@PipouwPieuw
Your page looks really neat!
I was wondering about the
loading="lazy"error in the HTML validation report. I got the same errors when I submitted my solution so I just removed the loading attributes from all my images. Does this mean the loading attribute should not be used or is the report tool simply not up to date ? Seems a bit odd. - @atmahana@PipouwPieuw
Hello and well done completing this challenge!
To answer your question, in this case you can use either flexbox or grid. I don't think there is any good practice that makes one of these better than the other.
Flexbox didn't work in your example because you also need to make sure that your element's height is at least equal to the window's height, as you did in your grid example. Adding
min-height: 100vhshould work. You would also need to setflex-direction: columnin this case because of the.attributionelement being a sibling of.card-container:body { display: flex; align-items: center; justify-content: center; flex-direction: column; min-height: 100vh; }Hope this helps. Keep coding :)
Marked as helpful - @arjuyyy@PipouwPieuw
Hello and well done completing this challenge, your component look good!
I have a few suggestions to further improve it:
- To handle the responsive behaviour of the image, instead of using two <img> you could use a picture element.
- A good
altattribute shouldn't just repeat the name of the product. Here you should really describe the image as you would describe it to someone that can't see it. - You could use a greater breakpoint to display the mobile version. Try viewing your page with a window width set to ~ 400-500px, you'll see it's all cramped.
- To make the button hover effect a little better you could use a transition (0.2s / 0.3s is usually enough).
Apart from these minor matters, you did a really good job! Keep going :)
- @ChrisLee-04-20@PipouwPieuw
Hello and well done completing this challenge!
This image is not purely decorative and is important to the page content, therefore it shouldn't be displayed as a background. I would use an
<img>here, or even better a<picture>.This may even fix your sizing issues. :)
Also you put a
<h2>right before the<h1>which is semantically incorrect. You could wrap the wordPerfumeinside a<p>or maybe a<span>instead.Keep going, happy coding!
Marked as helpful - @adityaphasu@PipouwPieuw
Hello! Well done completing this challenge, your page looks really neat! 🎉
To answer your question, I may be wrong but I don't think hover effects have any negative impact on performance. They are good for the user experience though, so ideally every interactive element should have one :)
Keep coding! 🧑💻
- @azammustafa66@PipouwPieuw
Hello, well done completing this challenge!
The
border-radiusyou set doesn't show because you gave apaddingto your image. Remove this padding and instead apply it to the element that contains the image, here.card.To create a hover effect you could wrap the eye image inside a
<div>, give this div a coloured background, center the eye icon inside using flexbox and use absolute positioning to put the div over the image. Then you give it anopacityof 0 which you switch back to 1 when the element is hovered, usingdiv:hover. This should work :)Hope this help. Dont hesitate if you have further questions. Keep going!
Marked as helpful - @marudever@PipouwPieuw
Hello and well done completing your first challenge!
To answer your question, I don't see anything wrong with using lots of classes. I'd even say it's best to target a class rather than a tag name like
div / h1 / petc.Frameworks like Bootstrap and Tailwind heavily rely on classes, same for the BEM methodology, and it's perfectly fine! Classes exist so they can be used this way. :)
Your component looks good both in desktop and mobile mode. There are a few HTML issues you could fix, everything is listed in the reports above and they're quite self explanatory.
Well done and keep going!
- @Walkwithrahul@PipouwPieuw
Hello @Walkwithrahul, well done completing this challenge!
- Your component looks good on desktop and mobile sizes but roughly from screen width 500px to 600px you can see the card overflows to the left. To fix this you can use a higher breakpoint to display the mobile version.
- To go further, you can add a hover effect with a transition on the cart button.
- Also I don't think the tag <figcaption> should be used anywhere but inside a <figure> element.
You did a good job otherwise, keep going! :)
- @michaelboateng1@PipouwPieuw
Hello 👋
It displays well on my phone so I guess you managed to make it responsive! 🎉 I can't inspect your code further right now though but I may come back later. In the meantime you may want to look at the reports above ⬆️ to improve your HTML structure.
Well done completing your first challenge, keep going!
Marked as helpful - @Luzefiru@PipouwPieuw
Hello @Luzefiru and congratulations for completing this challenge!
To answer your questions :
1. Your hover overlay method is fine! However I would change a few things :
- Switch the :before and :after to make the eye icon pass in front of the background. Or you could set
z-index: 1to the :before. - Try using a CSS transition to make the effect smoother. You could also set a transition to the title and author's name hover effects.
- Also, this is not about the hover effect but I wouldn't set the product image as a background. It's not a decorative image here, it would be good to make it accessible to screen readers by using an <img> tag and providing an alt attribute to describe it.
2. To make an absolute element take the full size of its parent, first make the parent's position
relative. An absolute element's position is calculated based on its closestrelativeancestor. Then you can make the element match its parent's boundaries like so :.element { position: absolute; top: 0; right: 0; bottom: 0; left: 0; }3. I wouldn't say it's a bad thing to use
background-size: contain, as long as the icon displays correctly. Here again I would have used an <img> tag but as it is a decorative icon, it's fine this way I guess. :)4. I don't think you can change the colors of a SVG with CSS if you insert them as background image or even with an <img> tag. However you can do so by inserting the raw SVG inside your HTML, then you'd have access to the elements inside. But I really don't know if it's a good thing to do. Another way to do this is by converting your SVGs into a webfont. The icons can then be inserted as characters and you can change their color and size using CSS. This process is a bit complex though. For beginners, I'd say your method is really fine. :)
I hope my comment was helpful. You did a good job, keep going!
Marked as helpful - Switch the :before and :after to make the eye icon pass in front of the background. Or you could set
- @AnaLuisaFav@PipouwPieuw
Hello 👋 Well done here, your hover effect looks neat!
I have a few remarks:
- You applied the equilibrium image as a background but as it is not a purely decorative image, you should use an
<img>tag instead. - You always need to have a
<h1>on your page. This adds meaning to your page and shows what the main topic is. Here, the<h1>should beEquilibrium #3429. You can of course keep the<a>wrapped inside or around the<h1>:)
Keep going 🎉
Marked as helpful - You applied the equilibrium image as a background but as it is not a purely decorative image, you should use an
- @AnaLuisaFav@PipouwPieuw
Hello Ana Luisa! Well done completing this challenge.
- I would replace
height: 100vhbymin-height: 100vhon the.homeelement. - Generally I would advise to avoid selecting an element by its tag name in your CSS. Imagine having to change your <p> into a <h2> or anything else for SEO purposes or whatever reasons, then you'd have to update both your HTML and CSS so your styles won't break. Using tag names is ok for global styling but otherwise using classes is generally best.
Anyway well done here, there is very little to say :)
Marked as helpful - I would replace
- @LoaiEsam37@PipouwPieuw
Hello Loai, well done once again !
I noticed a small issue with your
Proceed to paymentbutton. Currently only its label is clickable but ideally we should be able to click anywhere on the button to trigger it. This is because you applied the styles to the wrapper elementcard__accept--button. Instead you should apply them to the <a> inside.Otherwise your component looks good! Well done :)
Marked as helpful - @AnaLuisaFav@PipouwPieuw
Hello @AnaLuisaFav, well done completing this challenge! :)
To go further, here are a few suggestions:
- Instead of using two images for desktop and mobile and hiding them with CSS, you should look into the picture tag. It is very helpful once you know how to use it!
- You used a <h4> right before your <h1>. However, h1 should always come first, then you can use h2, h3 etc... but never skip a level in the hierarchy. This is important and gives your page a good semantic structure. Here your could wrap the word
perfumeinside a <p> or maybe a <span>. - Also I see a lot of
!importantin your CSS but this may be due to Bootstrap? You should usually try to avoid using this. The less importants the better :) - Maybe avoid setting your images width inside the HTML. I think your code would be easier to maintain if all styles were set into your CSS file(s). I don't know if there are best practices on this matter, it may just be a matter of personal preference.
Overall, you did a really good job! Your page looks good and clean in desktop and mobile sizes :)
Marked as helpful - @LoaiEsam37@PipouwPieuw
Hello Loai, good job completing this challenge!
Your page looks good :) Here are a few suggestions to further improve it:
- Instead of using 2 images for desktop and mobile and hiding them with CSS, you should look into the picture tag. It is very helpful once you know how to use it!
- I wouldn't set a fixed height to the card because you'd want it to adapt to its content. Imagine having to write a longer text inside, then the card should be able to expand.
- I don't think
perfume imageis a good alt attribute. Think of how you would describe the image to someone that can't see it. For exempleA bottle of Gabielle Essence Eau De Pafum lying between two leaf branches.
Overall I think you did well, and the mobile version looks neat too! Good work :)
Marked as helpful - @Rajoz-dev@PipouwPieuw
Hello! First of all, good job completing your first challenge!
Here are a few answers and suggestions that might be of interest to you:
- To center your box efficiently, you can use flexbox on your container instead of using an absolute position :
#container { display: flex; justify-content: center; // Centering horizontally align-items: center: // Centreing vertically min-height: 100vh; // To make the container at least as tall as the screen }- The font size and colors are given in the
style-guide.mdfile included in the challenge archive. You should check it out :) - Don't forget to add an alt attribute to your images, this is very important for accesibility purposes.
- Avoid using
<br>tags to skip lines inside a text. This could make for bad text display if the text container were to get narrower. - Use
max-widthinstead ofwidthon your .box to make it responsive. - You don't need to set a fixed height to your image. Setting
height: autowill make the image's height automatically adapt to its width. Also you can just set its width to 100% instead of a fixed pixel value. This way the image wouldn't overflow if the box were to shrink.
Overall you did a good job, keep it up!
Marked as helpful - @r-colleti@PipouwPieuw
Hey there, well done completing this challenge!
Your component looks neat, I love the loading animation and the tooltip following the hovering from bar to bar. :)
One thing you could improve is highlighting the current day dynamically (currently it's thursday but the wednesday bar is highlighted).
Anyway you did a really good job here 🎉
