Rực Chu Văn
@vanrucAll comments
- @sd3u16P@vanruc
Hi Dear,
Your solution look good on mobile phone, except the grid of images we should show on 1 column as the design.
Seem you haven't finish work on table and desktop. Please tell us what is the most challenge for you on this solution, so we may have some suggestion on it so.
Thanks and regards, Ruc
- @PaiKai-LeeP@vanruc
Hi Pai,
Nice to have some comment on your solution.
- Firstly, as our design the body of the page will have background color : #F6F5F6, by adding background color you will see your card easier(currently as you see, white background card look shorter than other block)
- Secondly, you may review the font size, font weight of each card text content.
- Thirdly, not all of image has ring, please double check.
- Lastly, you may try to implement style for tablet to match with the design.
Hope this help, Ruc
Marked as helpful - @AnkitSinghSenwalP@vanruc
Hi,
Well Done.
I just have only 1 comment. When user hover to the button, please change the text color to black.
Hope this help, Ruc
Marked as helpful - @xuanphu2701What are you most proud of, and what would you do differently next time?
- Discover variables in CSS
- Finish this much quicker than the last two
- My confidence gained so much!
- Start using semantic tag
- Currently not
P@vanrucoh, chào Phú!!!
Good job. Tuy nhiên mình có một vài comment nhỏ như sau.
- Bạn chưa load đúng font chữ
- Cho phần social link phía dưới bạn nên chuyển qua sử dụng thẻ <a> thay vì sử dụng button
- Hiện tại mình thấy bạn đang để style cho button khi active thì chuyển màu, nhưng nên chuyển nó qua khi hover.
Regards, Ruc CV
- @ReneOsvaldoCamachoFernandez
Four-Card-Section component responsive Next.js Tailwind CSS TypeScript
#next#tailwind-css#typescriptP@vanrucHi,
Congratulation you on finish the challenge.
I would like to have some comments as below:
-
seem you haven't implement for tablet screen size.
-
Desktop version Look good, except some element space is not pixel perfect. I think it will be better if you have figma file and follow the design.
Many thanks, Ruc
-
- @asmaaelbahrawi1P@vanruc
First of all, Congratulation.
Secondly, I would like to have some comment on your solution:
- you are missing design for mobile and tablet versions.
- On your desktop version:
-
One small note: you haven't define font-size for html tag, so it will be set as default 16px, it will affected to your calculation base on rem(like font size, width, gap, padding, margin...)
-
The first-p: Current font size currently is 36.8px, while design is 36px Font weight should be 275 Color is not follow design Line height should be 140%(50.4px) It should have letter-spacing of 0.25px;
-
The second-p: same as first-p, just different from font-weight
-
The desc-p: Color should follow our design: grey color #4D4F62 Line height: 140% And I think we shouldn't use <pre> tag for it, using <p> tag with proper width, padding, margin will be okay.
-
Your card container Display flex is correct But the flex item is a bit hard to control. I suggest some change like below:
a) card container: .container { display: flex; gap: 32px; align-items: center; justify-content: center; }
b) for flex item left and right:
- remove all margin: left, right, auto.
So your card will be more flexible.
The rest of small thing, you may try to do close as design as possible.
Thanks and good luck with your improvement, Ruc
- @Sage-25What are you most proud of, and what would you do differently next time?
What I am most proud of about this task is the fact that it made me realised that I had be coding wrong for a while, more like I have come to a single realisation that If I had used my divs well, I would have coded differently and efficiently in my past submissions.
What I'd do differently next time is ensure that I try different things and not be scared of breaking things on the process, it might be frustrating but it would help me learn something new.
What challenges did you encounter, and how did you overcome them?The only challenge I encountered on this task is that the image wasn't touching the floow of my flex regardless of how many times I increased the width, I wasn't sure what was wrong, the images I had used previously had never challeneged me this much before -I still haven't overcame it, I tried using rem's, percentages and still didn't find a way to scale it.
As a desiger, the fact that it wasn't aligning properly really disturbes me.
What specific areas of your project would you like help with?Currently, I would just like an honest review based on what I have done so far and how to improve my written code.
P@vanrucokay Chinedu Daniel Okeke,
Well done. I will help you to fix the issue of the image.
Firstly, you need to change style for main container. align-items need to change to stretch instead of center. so it will be: .main_container { display: flex; align-items: stretch; width: 40rem; background-color: white; padding-right: 2.5rem; border-radius: 1rem; }
Then the image, you will need to add height: 100%, and add object fit to it. .channel_image { width: 20rem; margin-right: 2rem; border-top-left-radius: 1rem; border-bottom-left-radius: 1rem; height: 100%; object-fit: cover; }
Hope this help, Ruc
Marked as helpful - @PrabinMhznP@vanruc
Hi Prabin Maharjan,
Nice to see your solution. I would like to have some comments on your solution.
A) Mobile Screen
- Image block
- Showing good, however as I see, you are loading same image as desktop version, while challenge starter provide us with image for mobile as well. You may enhance this part.
- Text block.
- As design we only have 2 fonts: Montserrat and Fraunces
- The gap between tag, product name, and product description should be 24px a) The h3 tag - perfume
- currently it load outfit font, even you define it as Montserrat
- font-size: 12px instead of 16px
- font-weight: 500 medium
- letter spacing should be 5 instead of 6.4 b) h1 - product name
- font family: correct
- font-size: 32px instead of 36px
- color: correct c) p - product description
- almost correct
- line-height: 160% of 14px = 22.4px
- Price box
- Gap between promotion price and original price should be 16px instead of 2rem a) Promotion price - green
- font-size: 32px instead of 30px
- color: #3D8168 b) Original price
- Font size: 13px
B) Tablet and Desktop screen
- Look perfect except for general width should be 600px
- Refer to the mobile screen's comment to make change on text property as well
Thanks and best regards, Ruc
Marked as helpful - @PaulgultiP@vanruc
Hi Paulgulti,
Nice to see your solution on peer review. Well done with your tailwind implementation.
There are just few comments from my side.
- You can custom your tailwind:
- font
- color by using @theme {
} 2) Preparation section
- should have border radius: 12px
- Responsive design
- seem you did not implement layout for mobile screen yet.
Thanks and regards, Ruc
- @lingowmxWhat are you most proud of, and what would you do differently next time?
Im getting use to the sytles and luckly im remembering some stuff
P@vanrucHi Caleb,
Well done and nice to see your solution.
There are some of my comment on your solution
- You forgot to define background color for the body
- Space between block, content are not match with challenge design
- You forgot to add font family for text.
I think you are able to do it as perfect as the design.
Once again, Congrat.
Marked as helpful - @CodeLikeAGirl29What are you most proud of, and what would you do differently next time?
I'm most proud of the clean and modern design achieved using MDB Bootstrap. The profile card's responsiveness across various devices and its aesthetic alignment with current design trends are particular highlights. The integration of animations and hover effects also adds a dynamic touch, making the user interaction more engaging.
What challenges did you encounter, and how did you overcome them?Next time, I would focus more on optimizing the loading speed of the profile card. Although the design is visually appealing, some of the images and animations could be optimized to load faster. Additionally, I would like to experiment with more advanced CSS techniques and JavaScript to add interactive elements such as a collapsible bio section or real-time data fetching for dynamic content.
What specific areas of your project would you like help with?I would appreciate assistance with the following areas:
-
Advanced CSS Animations: While I've implemented basic animations, I would like to learn more about creating complex, yet smooth animations that enhance user experience without compromising performance.
-
JavaScript Integration: Guidance on integrating more interactive JavaScript features, such as dynamically updating content based on user actions or external data sources, would be valuable.
-
Accessibility Improvements: Ensuring the profile card is fully accessible to users with disabilities is important. I'd like help with implementing ARIA roles and ensuring keyboard navigation is seamless.
-
Code Review and Optimization: A thorough review of the codebase to identify any potential improvements in terms of performance, readability, and maintainability.
By addressing these areas, I aim to enhance both the functionality and user experience of the profile card project.
P@vanrucHi Lindsey Howard,
Congratulation!!!
Honestly, you have done very well the challenge. All the flex box item are in ordered, good structure, correct width and gap...
However, in my opinion, there are 1 place which need to be improved.
It is the social link block. Cause there are links to social profiles, so I think we could use anchor tags instead of button. So, I suggest the structure for that part is:
<ul> <li> <a></a>and also, you forgot to add hover effect for each item.
Thanks and best regards, Ruc CV
-
- @manyolie3P@vanruc
Hi,
Congratulation!!!
There are my comments on what you have done.
Parent box
With is not match
design: w: 288px, padding left and right: 16px
your: w: 300px in total
Missing box-shadow style
Border radius:
Design: 20px
Your: 15px
The QR image
size:
design: w: 288px, h: 288px
your: w: 270px, h: 285px
border radius:
design: 10px
your: 15px
Text:
need to check in detail of font-size, line height, spacing and color.
Thanks and best regards, Danny
- @LEstebanRP@vanruc
Hello,
Well Done!!!
- Look like general padding is mismatch and the image should align with the content below it.
- Text color, font-size, line-height need to define correctly as well.
Thanks and regards,
Marked as helpful - @AnshShrivastava70P@vanruc
Hi AnshShrivastava70,
well done!!! However I have some comment as below:
- vertical alignment should be middle.
- parent box dimensions should as below:
- width: 288px
- padding top: 16px
- padding right: 16px
- padding bottom: 40px
- padding left: 16px
- border radius: 20px
- seem box shadow is not match
- image element
- should not have margin
- border radius 10px instead of 8px
- Text box - title
- Title font size: 22px instead of 24
- Line height: 26.4px
- Color should change to: #1F314F instead of black
- description text
- font-size: 15px
- line-height: 21px
Marked as helpful