@lewmas9152
Latest solutions
Latest comments
- @d8701a
Just try to center the text in your sections, especially headings. You can achieve this by setting the whole section to display: flex or you can try to center it with margins or translate method, it's up to you.
Also try to use picture instead of img elements for the cherry and orange pictures. Using the <picture> element is great because you can use images of different dimensions depending on the screen size. In your case, I think you used the image intended for a mobile version for your desktop version as well. For example, if you compare the mobile version of the orange picture, you see it has more height than the one made for desktop, that one is shorter.
The rest of your project looks good, keep up the good work!
Marked as helpful - @david-tejada@d8701a
Hello and congrats on completing this challenge!
The flipping card idea was awesome, it looks smooth and much more elegant than simply replacing one card with a thanks-card, although both solutions are correct and acceptable.
You chose a rather unusual solution, with rating buttons 1-5. Most of the solutions I saw here were just adding a button element in HTML and then styling it as necessary in CSS. You decided to go with radio buttons instead, which maybe generates a bit more HTML code but in the end, if it gives you a desired result - go for it!
Your code seems alright to me. Maybe try using more descriptive names for icons and images. For example: <div class="star"> <img src="images/icon-star.svg" role="presentation" /> </div>
If someone else gets to work on your project, the code is more readable and understandable if the classes have descriptive names. For example star-icon would be more appropriate in this case, so anyone looking at your CSS will know what this class does. If you only put div class = star, it's hard to see without checking the HTML if that star is an image, a logo or an icon.
However, your solution is very interesting, your code looks good and I think you are definitely on the right path, just keep it up! :)
Marked as helpful - @DarkGamerXDOFF@d8701a
Hi!
Using flexbox to center elements or for layouts in general is a good idea. So you made a good call when you decided to use it in this case. There are other ways of horizontally centering elements, such as using margins: (margin: 0 auto), using transform: translate(-50%, -50%) but this also requires an element to be absolutely positioned so I'd avoid this one, using grid and of course - using flexbox. For a simple layout like this, flexbox seems like a perfect use case.
When it comes to managing the card size, I believe the priority should be its responsiveness. It should be able to change it's dimensions depending on the screen size. For this, try learning more about setting width with one of these three things: min(), max() and clamp() function. For more information on these, check this article: https://web.dev/min-max-clamp/
- @MatejBumbera@d8701a
Hello and congrats on completing this challenge! I checked your code and overall, it looks very good to me. It's clean and precise, also you didn't repeat yourself, so that's also great.
I noticed two things, so I'll write them here, but they are not mistakes, just maybe little improvements to be made.
- First: try using semantic HTML elements instead of generic containers. So you could replace <div class = "main"> with simply <main> or add a class to it, for example <main class = "yourclasshere">.
There are a couple of reasons why this is better and one of them is that <main> already exists in HTML, so you can use it. It improves accessibility of your website, code readability, and can definitely improve SEO ranking for your website as well. Another semantic HTML elements are: <nav>, <article>, <aside>, <section> etc. Try integrating them into your code and give them priority over custom div elements whenever possible.
- And second thing - try to avoid using absolute units such as pixels. They are acceptable for setting paddings and margins (although some people use rems or ems for them as well), but for font-size try to use rems. They improve mobile responsiveness and overall using absolute units is a bad practice now.
Other than that, awesome use of flexbox, awesome use of width and max-width on a container and overall, just keep learning, you are on a good path!
Marked as helpful - @siavashgholami@d8701a
Hello and congrats for completing this challenge!
Now to answer your questions, you did add a box-shadow and that's completely fine, as a matter of fact your version looks more polished, and I like it. I don't think, however, that you needed to add box-shadow property at all, I believe this card doesn't have it set, but in the end, it really is a matter of preference, not a mistake. You can simply remove it or decide to keep it, both ways are fine.
When it comes to centering the image in the mobile view, it's probably due to margins and padding. I can see in your code that you used the mobile-first approach, and that's commendable. So you set a media query for screens larger than 400px where you change the width of the container, but you didn't change all the paddings and margins. Usually, when we resize the size of a container/wrapper, margins and paddings need to be adjusted as well.
Also if you want to have better control of images, make them responsive etc., you can try wrapping them inside a .div, setting dimensions on that .div and then just telling the img to take the whole width of that .div. Like this:
<div class = "image__container"> <img>your image goes here</img> </div>And in CSS:
.div { width: 420px; height: auto;
img { width: 100%; height: auto; (if you want an image to preserve an aspect ratio) object-fit: cover}
This way you are telling the img to take 100% of its parent container, which is the .div container and it's width will be 420px. In case you decided to set the width of your image to 50%, then it would take only 50% of 420px. It makes images responsive and easier to manipulate.
Also by limiting the container size, your image for this project would've been smaller.
Other than that, I see you are already familiar and comfortable using custom properties/variables and rems as relative units which is awesome! Keep up the good work and just keep on learning!
- @3p0na@d8701a
Hello there! Congrats on having completed this challenge, awesome work!
To answer your question, yes, definitely you could use sub-divs. I saw many developers use them.
So your whole card could have been wrapped in a .box_wrapper div. Then inside, you could've created .image_wrapper and .content_wrapper. Inside the .image_wrapper would be the QR code image, and within the .content_wrapper you could have put the h2 and p elements with their content. As a matter of fact, this is how I did the challenge.
To summarize, everything would be contained in a .box_wrapper, which would be the parent container, and you could set it to display: flex, set its width etc.
Marked as helpful
