Sarvotham Gowda
@sarvothamgowdaAll comments
- @SaikiranVullaSubmitted about 2 years ago@sarvothamgowdaPosted about 2 years ago
Hi Sai Kiran,
Good start.
- You can center the container by using flex in the body and its properties. Learning flex and grid would help you to solve most of the challenges. Instead of using <br> make use of the margin property to create space between elements.
body { min-height: 100vh; display: flex; justify-content: center; align-items: center; }
- As a good practice use:
<html lang="en"> <head> <meta chartset="UTF-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> </head> </html>
You can learn more about the attributes and tags here https://www.w3schools.com/
- The font and colors are not as per the design. So you really need to read the relevant style files provided as a part of this challenge.
Hope this helps!
Marked as helpful1 - @StephenAlcantaraSubmitted about 2 years ago@sarvothamgowdaPosted about 2 years ago
Hi Stephen,
Good job. You can add a few lines of code to improve it even better. For example:
- Since, you did not define the viewport height the card has flushed all the way to the top.
body { min-height: 100vh; }
This will give enough room to center your container.
- For letter-spacing in the span inline element. Defined property is invalid.
span { letter-spacing: 15px; }
- For the desktop version the image dimensions and size of the card are different. You can also make use of @media queries to build a responsive mobile card which allows the child items to stack on top of each other.
Hope this helps!
0 - @nuwanchandrasekaraSubmitted over 2 years ago@sarvothamgowdaPosted about 2 years ago
Hi @Nuwan,
Good start. Even though I'm still learning and new to HTML and CSS I would like to share some feedback based on my learnings.
- For the body consider using flex and its properties to align the container to the center of the page. Consider using this:
body { min-height: 100vh; display: flex; justify-content: center; background-color: hsl(212, 45%, 89%); }
-
Consider using semantics elements (for eg: <picture> ) which clearly defines its content.
-
I think the path for the image is wrong hence the QR code image is not showing. While I add images I always declare its width and height in the img markup. And use width of 100% and height auto in the CSS to make it responsive. You can also consider using Object-fit: contain; to retain its original aspect ratio and it will fit within the given dimension.
Since, you have declared img as a class="qr-code" use that in your CSS rule.
<img src="<path> alt="QR-code-image" width= " " height " ">
.qr-code { width: 100%; height: auto; padding: 1rem; border-radius: 15px; }
Marked as helpful0 - @MOHITBILALASubmitted over 2 years ago@sarvothamgowdaPosted over 2 years ago
Hey Mohit,
Good Job! Your solution is almost close to the design. One thing I noticed is that you have forgotten to add border-radius to the container.
.container { border-radius: 5px; }
Happy coding!
Marked as helpful0 - @Abdelrahman0KhaledSubmitted over 2 years ago
how can i sit background imgs better? **how can i make this project with best practice ? ** I am still learning ..all feedback is welcome
@sarvothamgowdaPosted over 2 years agoHey, good job!
Especially on aligning the testimonial cards as per the design. The only thing where you can work on is the background image position. Here is my solution.
- mobile version:
body { background: url(./images/bg-pattern-bottom-mobile.svg) no-repeat fixed bottom right, url(./images/bg-pattern-top-mobile.svg) fixed no-repeat top left; }
- desktop version:
body { background: url(./images/bg-pattern-bottom-desktop.svg) no-repeat fixed bottom right, url(./images/bg-pattern-top-desktop.svg) no-repeat fixed top left; }
Marked as helpful0