Looking to hire developers?
@xavCS
@LVHuertas
Submitted
I'd love feedback.
In this case, I had some troubles with images on how to put them in the right way. Also, I think button has some problems.
I didnĀ“t understand what I need to do with hover and focus states, so I put a hover in button with only a slightly opacity.
@xavCS
Posted
Hey, nice attempt at this challenge, a few things:
box-sizing: content-box;
This declaration in your CSS is not really needed since the browser uses this by default and actually I'd recommend you go with this declaration instead:
html {
box-sizing: border-box;
}
*, *::before, *::after{
box-sizing: inherit;
}
https://developer.mozilla.org/en-US/docs/Web/CSS/box-sizing https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_block A couple of useful MDN docs on this topic
Your button is fine it just wanted a darker colour in the hover state, but using a styled <a></a> tag is better as it will link to a different place on the site to buy this product.
I believe your image issue is coming from setting explicit heights and widths as well as the fact that you're trying to style the image whilst using the background-image property.
It would be easier and more readable if you just import your image on the site using the <img></img> tag and set a max-width: 100%; on it. This will make it responsively sized by default as it will take as much space as it can within the container.
Hope this helped.
Marked as helpful
@PowerCreation
Submitted
I would love to get feedback about best practices and how i can improve my code. Have a nice day!
@xavCS
Posted
Hey, nice job - I do have a few comments on your code that could hopefully help you out.
<div class="card-content">
<h3 class="desc">PREVIEW</h3>
<h1 class="title">Gabrielle Essence Eau De Parfum</h1>
In this codeblock you've started with a h3 then gone to a h1, I would emphasise that you try to stick to the order of headers, where h1 is top level and h3 is 2 sub headings lower. This is more of an accesibility thing for people in general and more specifically screen readers! Using headers out of order breaks their semantic meaning so you'd really rather avoid that. For styling purposes, just resize and redesign with CSS.
Small MDN docs page on headings https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements
Another thing is your button, for this small project it's fine but you can imagine it would probably lead somewhere to buy the product, so using an anchor tag would be better to get that href in for effective browsing
Your CSS is nice and tidy which is always a plus! Hope this helped
Marked as helpful
@VittorioDL
Submitted
I appreciate any suggestion to improve my code! Feel free to tell me what I could have done better. I focused mostly on the responsive aspect of the card. The change the layout of the card I applied the grid display on the desktop version and changed the image
@xavCS
Posted
Overall I think you pretty much got the design and placement bang on, just a few margins and line spaces that could be adjusted
I think if instead of setting the display:none on the relevant image when the viewing mode changes, you could instead use a <picture></picture> element. I'm suggesting this because the way you have it set the browser still loads both images, which isn't a good use of bandwidth, with the picture element you can dynamically tell the browser which one to load instead of loading all available images.
Hope this was useful
Marked as helpful
