@roychanduSubmitted 4 months ago
P
Marcus
@marcus-hillAll comments
- @marcus-hillPosted 18 days ago
Hi @roychandu, good job with this - I think you've made a good effort but there is definitely scope for some improvement. I am unable to access your GitHub repository so I am providing this feedback visually & with access to the dev tools:
- There does not appear to be a max width on your card in
rem- instead it is set to60%which makes the card incredibly large on bigger displays, which isn't good. - As this happens, the profile picture and author's name overflows out of the card with the text also scaling incredibly large - given you have a font size set in
vm. I do not see why you have the font size set invmwhich is a percentage of the viewpoint width. I would recommend usingremfor your fontsize, which scales with the user's browser default. - On mobile, there appears to be a significant amount of white space at the bottom of the card. I can see you are utilising flexbox with
flex-direction: column- however there appears to be no use ofjustify-contentoralign-itemswhich could have created the required spacing between these elements, such as the use ofspace-between. - On pressing the share button in mobile view, there is no button visible which allows for it to be toggled away again.
- There also appears to be no max width on the image, which will continue to scale so large or so small (to the point that little of the image is visible) as I switch to mobile view which is quite a problem for this responsive design.
With a few improvements, this design will be much better. Remember to allow access to your Github Repository (I am assuming you have forgotten to make it public).
0 - There does not appear to be a max width on your card in
- @MarenOelixtownSubmitted about 1 month agoWhat are you most proud of, and what would you do differently next time?
👉I tried to get a smooth transition between the different output sizes. With trial and error I came to the conclusion that if you give an element a max-width and centre it with margin-left and margin-right ‘auto’, you could already achieve a lot. In addition, clamp()-function helped me not only with font-sizes, but also with the scaling of images and spacing.
🔎 In the future, I would also like to learn more about tricks and tips for smooth transitions between different output devices to achieve a responsive design.
What challenges did you encounter, and how did you overcome them?👉 First time that I used the 'background-image' with the linear-gradient and also first time I created a section devider like this.
- background-image - This helped me for designing the footer-background.
👉 I liked using the picture-element in this way for the desktop media-query without css. Using the picture-element and the srcset attribute also ensures that only the necessary images are loaded depending on the screen size.
What specific areas of your project would you like help with?I have spent some time thinking about how to structure the html and also how to use BEM correctly. There is certainly room for improvement. I am looking forward to your suggestions.
It would be interesting to know whether all images (with the exception of the logo) should be considered decorative. In this case I did it, and provided a null (empty) alt text (alt="") so that they can be ignored by assistive technologies. What do you think?
@marcus-hillPosted about 1 month agoHi @MarenOelixtown, I think you've done a great job with this. It matches the design and looks amazing. The CSS looks pretty clean.
However, there do appear to be a couple of issues with the responsive nature of the design. At approximately 1200px, the content at the top is very much squashed together and not vertically aligned. Also, the "Smarter Meetings" header looks like it is meant to be hidden for tablets, as per the figma design (but you may not have noticed that).
Good luck in the future!
0 - @meghaspatil1Submitted about 1 month agoWhat are you most proud of, and what would you do differently next time?
I loved the way to takle testimonial with grid and more knowledge on CSS grid.
What challenges did you encounter, and how did you overcome them?Challenges came across arragement , tried most of way like grid area, grid column and row count.
What specific areas of your project would you like help with?confusion with handling css-grid-area and grid-column or grid-row.
@marcus-hillPosted about 1 month agoHi @meghaspatil1, good job with this, it looks good but here's some feedback for you:
- Your solution looks much taller than the design. Perhaps you could set a
max-heighton your testimonials container? - Doesn't look like you have loaded the required font, although it is referenced in the CSS
- There is a lack of responsiveness in this design. On smaller screens (and mobiles), your container with all testimonials is pushed to the right of the screen and half of them are hidden. Might be worth seeing if you can center the cards any better.
0 - Your solution looks much taller than the design. Perhaps you could set a
- @Djamel1133Submitted about 1 month agoWhat are you most proud of, and what would you do differently next time?
I did it with flexbox (not with grid) and started using logical units instead of max-width and max-height. I used a mobile-first approach, which saved me a lot of time. I'm happy to follow my learning path and to be with you 🥰.
What specific areas of your project would you like help with?Responsiveness again
@marcus-hillPosted about 1 month agoHi @Djamel1133, good work with this! I like your use of flex box in order to create the finished design, which works as one of the multiple ways of completing this challenge.
Here's some feedback for you:
- The coloured line at the top of each card appears to have been done differently for the Supervisor card compared to the other three. I would stick to using
border-top:to achieve this (as you have done with the Supervisor card), rather than utilising the<hr>element and then sticking aborder-top:on top, which overlaps with theborder-radius. - The box-shadow doesn't appear to match the design brief, it was specifically looking for one on the bottom of each card.
- It's great you are using responsive CSS units for the margin and font sizes, which scales well with a user's default browser font size.
- Did you need to make each card itself a flexbox (which you've done by targeting
.cards div)? By taking offdisplay: flexon these, the only element out of place is the icon which could then be aligned to the right in another way? The elements in the card are already block elements and so will already stack like a column, questioning whether flexbox was needed.
Good luck in the future!
0 - The coloured line at the top of each card appears to have been done differently for the Supervisor card compared to the other three. I would stick to using
- @nebiyoudawitSubmitted about 1 month ago
- @kalebbirhanuSubmitted about 1 month ago@marcus-hillPosted about 1 month ago
Hi Kaleb, I'm unable to access your Github Repo (perhaps it isn't public?), however I can provide some feedback based on your live preview link and the dev tools:
- Good start with the mobile design first, I can see this is where you've put your effort and began with mobile design, which is best practice.
- Good use of some semantic HTML, however I think you could have used more, such as the <article> tag and also the <section> tag.
- On a desktop, your solution does not meet the design and is far too wide, extending nearly to the left and right hand side of my screen. Whilst it works on a smaller screen size (of 1000px), it begins to get far too big on anything larger. From looking at the dev tools, there doesn't appear to be a max-width set on the .total class div, which is causing it to grow and grow in size until the content is stretched way beyond necessary.
Keep up the good work and I am sure this will be much better with a few changes.
0 - @KristinaHorbenkoSubmitted about 1 month ago@marcus-hillPosted about 1 month ago
Great job Kristina, your preview matches the design files perfectly and I think the flexbox use was a great way to do it. However, at 375px (mobile size) the social card is very squashed. Perhaps a media query could solve this? Keep up the good work!!
0 - @begli-amanovSubmitted about 2 months agoWhat are you most proud of, and what would you do differently next time?
Everything is outlined in the Updated readme file.
What challenges did you encounter, and how did you overcome them?It was really annoying to tweak responsive text without media queries. Need to find a way to make it more effective. The approach was like from 90s. To much manual setup. Or maybe we are just too lazy?!
What specific areas of your project would you like help with?I am mostly interested in if my html and css semantics are robust enough and is my approach with cal() function good enough for responsive text on the page.
@marcus-hillPosted about 1 month agoHi Begil, great job on the design! I am only starting out but here is some feedback I have:
- Even with the use of a media query, your card is still too big for a small viewport (below 370px) and leads to the left hand side of the content being hidden.
- I like your use of CSS variables, I haven't used these before but it prevented a lot of repetition of colours and styles.
- I think the way you added the hover effect was quick by selecting multiple classes, but I don't believe this was required as per the design brief (only the name should apply)
Good luck with your learning, you're doing a great job!
1 - @JammiMSubmitted about 2 months ago@marcus-hillPosted about 2 months ago
Hi James,
The design looks good and I think you have done well with that. However, the obvious issue is the positioning of the card which is not currently in the middle of the screen. Setting a min-height on the body element should solve this problem I believe.
Hope that helps!
Marked as helpful1
