Design comparison
Solution retrospective
que podria mejorar en este proyecto? que otras cosas podria agregar o usar?
Community feedback
- @rmartin93Posted over 1 year ago
I would focus on simplifying the html and css. You shouldn't need as many p tags as you have, and you're making a lot of things display flex when a text-align center would be just fine.
Example html simplification
<div class="marco"> <div class="card"> <div class="card-image"> <img src="./images/image-qr-code.png" alt="QR"> </div> <div class="card-body"> <h2 class="oscuro">Improve your front-end skills by building projects</h2> <p class="frontend">scan the QR code to visit frontend mentor and take your coding skills to the next level</p> </div> </div> </div>
From there, it shouldn't take much CSS to make the h2 and p tag look how you want. I would try just using text-align and play with that a bit.
Happy coding!
Marked as helpful0 - @grace-snowPosted about 1 year ago
Hi, sorry it's taken a while for me to get to this.
I think you need to refactor this solution, changing quite a lot of HTML and CSS to get it much closer to the design. This would not pass in a work environment as it is at the moment.
- Yes simplify the HTML, but go further than the suggested above. This is ONE component, with an img, heading and paragraph inside. That's really it. You don't need all those extra divs or paragraphs.
- Always use a
main
landmark on every page or component demo you ever build. In this case themarco
div can become amain
. - Use the background colour provided in the style guide
- This component does not have a border in the design, it has a subtle box shadow
- The component must not have a width or height. It should have a max-width only and this should be defined in rem, not px
- The component should have some padding. The image should not have margin, except optionally a margin-bottom.
- The img should not have a width. You should always include a modern CSS reset at the very start of styles. One of many things this would do is set img to
display: block; max-width: 100%
. That - and border-radius - is all the styling that this img needs - Border-radius should always be a fixed value, not percentage. Using % will result in really strange distorted radii on non-square elements
- The image needs a proper alt description. It should say what the image shows and where this code goes. Like "QR code to FrontEndMentor.io"
- The image does not need a wrapping div, nor does the text content in the card. And neither would need display grid or flex. Don't add complication for no reason
- The HTML you choose is extremely important for communicating what this content is. Read my post on choosing HTML elements for more on this. It's invalid to have all the paragraphs as you are doing at the moment. As said previously, this is just an image, heading and paragraph
- Font size and letter-spacing must never be in pixels. This is extremely important, and your sites will be inaccessible if you don't follow this recommendation
Good luck. I'll look at this again once you've refactored it, but won't leave comments on other solutions until then
0 - @rmartin93Posted over 1 year ago
The text should wrap naturally based on the width of the container.
0 - @LuccaMauroMolinaPosted over 1 year ago
para no poner tantas etiquetas p como puse antes, como hago para acomodar bien los textos como en el design
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord