@DuyTM0508
What challenges did you encounter, and how did you overcome them?
Using grid to position 4 card to the correct position in layout
Latest solutions
Latest comments
- @dali987
Hi!
Your project closely matches the design and works pretty well! Works both on desktop and mobile. I am going to list some suggestions:
- Instead of giving an unresponsive or static unit (like px), use a responsive unit such as rem. This tech allows you to set 1 rem to whatever the amount you want (in this case, 13px), and you can change it whenever you want. This is mostly useful when using media queries, since all you have to do is make the 1 rem into a smaller amount, and all fonts will shrink. All you have to do is change the font-size of the HTML element, and this will set the rem unit.
- there is something in grid called grid-template-areas which allows you to name places with only text and then all what you have to do is give the elements the right name, this is extremely useful here since it will take few lines and does everything that other grid methods do (btw its the intedned thing to do in this project)
- using these 3 lines:
background-image: url(images/bg-pattern-quotation.svg);/* set the img as background */ background-repeat: no-repeat; /* removes the repetition of the img (so there is only 1) */ background-position: top right 50px; /* positions the img */You can position the img instead of making an entire element and showing it and hiding it, these 3 lines will do the job.
That's all, I hope you find this useful.** Good luck!**
- @devlunch4@dali987
imma get straight to the point because im tired of typing comments ;-; you should use grid here, by making 4 rows and 3 columns, giving each card the row and column and there you go all of them gets the same size and the perfect position and by that you get rid of alot of janky code and issues. Also in the cards the text is on the left side while the img on the right (the text in the cards shouldnt be in the center), you shouldnt do 2 seperate texts at the top, 1 text and for the thicker one, add the b element which is intended for making the text bold. Also, you can remove the main element and use the body instead to center the elements. Also use responsive units like rem or em instead of static pixels (search about them)
(sorry if the comment isnt good but im really tired 😔) hopefully you find this useful!
- @Shashank-993@dali987
Hi! Your project is great, nice work! I got some suggestions for you. For HTML: Instead of using the figure element and hiding one image to show another, you can implement the new knowledge and use the picture element, which allows switching images based on the screen size. (side note: this project is made to teach you how to use this element and other image properties)
- Many elements can be overlooked. For example, the main element to center the card, using the body will work perfectly. Or here:
<div class="content-container"> <div class="content"> ... </div> </div>You can get rid of the content-container because it has only 1 element and won't affect the layout
For CSS:
- The sizing is quite big due to this line:
font-size: clamp(1rem, 2.5vh, 2rem);By changing the HTML font-size, you change the rem value. And here the sizing is too big, so changing some numbers will mostly fix the issues, and if it didn't work, just put 14 px there (following the style guide) and everything should be fine. (btw you're not meant to do clamp in HTML, only in texts in this project since the guide did give a specific number, but feel free to keep it or replace it since it will improve the responsibility if the numbers are correct)
-
the card's responsiveness is great but sometimes it doesn't look good (for example when the screen width is 1000) due to the css approach which requires a lot of media queries and even so won't look polished, so finding another approach is better, instead of using grid which will lead to countless complications, you can use flex which is simpler, requires less code and will work perfectly. You only need 1 media query when the screen size becomes smaller than the width of the card (to avoid ugly look), you will change the row direction to list the items horizontally, and make the image take 100% of the width instead of 50%.
.content { height: 100%; background-color: var(--white); display: flex; flex-direction: column; justify-content: space-around; }Here, you don't want flex since by default the element will be listed one by one horizontally, and each element has specific space from the other (margin or padding), so remove the flex stuff and adding margins or paddings to the elements is better
- Usually, you aim for code with fewer lines and simple logic. If you don't achieve that, it's okay, but at least you need to try and minimize the code. And media queries take a lot of lines, so you should always use them as a last resort when you can't do anything to make the site responsive, or you will add something.
Overall, the work done is really good! Keep up the good work! (i accidently submitted the comment before completing it 😔)
- @77KromoWhat are you most proud of, and what would you do differently next time?
transitions
@dali987Hi! Your work is excellent, but there's some stuff to add.
For HTML:
- Sometimes, you don't have to add a div element to every section or element. For example, when you did a div for the image, since it's only 1 element, it won't affect the layout, so adding it is just more code, making it look janky. Also, if you decide to add a div element to every part of the project to make it organized, it will lead to more CSS styling.
For CSS:
- Another note is that instead of giving an unresponsive or static unit (like px), use a responsive unit such as rem; this tech allows you to set 1 rem to whatever the amount you want, with the ability to change it whenever you want. This is mostly useful when using media queries, since all you have to do is make the 1 rem into a smaller amount, and all texts will shrink. All you have to do is change the font-size of the HTML element, and this will set the rem unit.
- Try to find the right properties so you don't have to add a lot of media queries, like using responsive units like rem, using max-width or min-width, something like that.
- Follow the example using the style guide file, which provides you with the colors, fonts, sizes, and font weights. Also, you forgot to make the image have the full screen width when it's 375px or less.
Overall, your work is great! Hopefully, you find this feedback helpful, Good luck!
- @shalforb@dali987
This is great! The website's responsiveness is pretty good, and the CSS work is clean! besides the slight changes from the original example (like making the list squared or forgetting to make the img round...) its really good! Keep up the good work man!
- @Lalalaloo@dali987
Hello!
you did a really great job there, i really don't have much to say.
these are some things that could be improved:
-
instead of precisely adding margins and padding with specific amounts and directions, try to add as few of them by putting a few in the right places (since i see a lot of margin setting)
-
small note is that you styled the same exact element twice (at line 93 and 106) putting them together is better for less code
-
another note is that instead of giving an unresponsive or static unit (like px) use a responsive unit such as rem, this tech allows you to set 1 rem to whatever the amount you want (in this case 14px) and you can change it whenever you want. This is mostly useful when using media queries since all you have to do is make the 1 rem into a smaller amount and all fonts will shrink. All what you have to do that is change the font-size of the html element and this will set the rem unit.
thats all! Good job in making this amazing project. Hopefully you find this feedback useful! Good Luck!
Marked as helpful -
