Feng
@niuguyAll comments
- P@danmlarsen#react#tailwind-css#typescript#vitest#motionP@niuguy
great solution with tests , thanks for sharing
- @dullarzeeP@niuguy
The layout looks great, and dark mode is working well. Just a couple of thoughts:
- On mobile, the search and filter elements seem to overlap.
- Should the border countries on the detail page be clickable links as well?
- @TrEv0rRrRrP@niuguy
All looks good to me. I learned quite a bit from your solution, thank you
- @ArcloanWhat are you most proud of, and what would you do differently next time?
It was really fun to build a page with state using React. It is the first project where I used state.
What challenges did you encounter, and how did you overcome them?I had a lot of trobule making the dialog on mobile version the same as the design. I endend up making it a little bit different. I used inset-0 on the dialog but it wasn't placed on the entire viewport. I didn't understand why. Instead I had a lot less trouble with vite deploying the site with github pages so it was a real relief.
What specific areas of your project would you like help with?Any help on the dialog, how to make it look similar to the design is really appreciated. It didn't respond like earlier projects where I built the mobile nav with ease using a fixed element and using inset-0. I didn't understand why it didn't work this time. Any other suggestion on my approach with state or anything else is really appreciated. Thank you :)
P@niuguyThe solution overall looks excellent. I noticed a few minor issues to address: on desktop, the '+' and '-' icons on the 'add to cart' button appear to be switching positions. Additionally, it would improve user experience if the confirm order modal could be dismissed by clicking anywhere outside the modal itself.
After all, congratulations on finishing this one! this is not easy, isn't it?
Marked as helpful - @Chayapol-cP@niuguy
all looks good , code is well-structured and readable 👍
A small enhancement could be setting a default tip rate (e.g., 0%) for non-US users:)
- @ArcloanWhat are you most proud of, and what would you do differently next time?
This is the first project I built with react. It was really fun. I find react really enjoyable!
What challenges did you encounter, and how did you overcome them?Nothing in particular.
What specific areas of your project would you like help with?I don't know if it is correct to leave an empty div to let it take the role of root for a react app or is a better way from an accessibility/pratical point of view. Any suggestions are really welcomed! Thank you
P@niuguyIt's good practice to set up components and keep the codebase clean. However, I'm unsure whether it's a good idea to place the components in the assets folder.
Marked as helpful - @zahraabelluP@niuguy
The layout looks good but the script seems not work。
e.g.In the script.js file, there are several missing closing brackets for event listeners and functions
after all this is not react, maybe it's easier if you try react
- @jasterpepito11What are you most proud of, and what would you do differently next time?
I made it to behave like it was fetching the data from an API with the help of tanstack query and added a skeleton component that will display while the data is still being fetched.
What challenges did you encounter, and how did you overcome them?I had a small issue with how to dynamically add a class based on the activity type(e.g. "work") and I have created css variables for each specifically like in
className={bg-${activities?.title}}
, so I can correctly display the correct icon for each activity component. As it is, I have seen the issue that Tailwind has no way of understanding string concatenation or interpolation and instead scans your source files as plain text. So I have come up with the solution like indicated here: Always map props to static class names
What specific areas of your project would you like help with?Any feedback will be greatly appreciated! Thanks!
P@niuguyall looks good to me. thanks for sharing a full functional app with tanstack query
- @FaojulazimP@niuguy
All looks good to me, it's not react though
- @tamara-tgodWhat are you most proud of, and what would you do differently next time?
I'm proud of the fact that I wrote the JavaScript logic myself
What challenges did you encounter, and how did you overcome them?The use of transitions in CSS
What specific areas of your project would you like help with?The CSS styling of the active state of the share button
P@niuguyIt looks good on desktop except the vertical positioning, if you can have a further look at mobile version the share section seems broke.
- P@AnonymousCoder323P@niuguy
Everything looks great! Astro seems like a perfect fit for this project—thanks for sharing the code!
- @MomonkueiWhat are you most proud of, and what would you do differently next time?
Practice using the same component to handle different styles
What challenges did you encounter, and how did you overcome them?Using different conditions in the same component
What specific areas of your project would you like help with?The height of my second line is not consistent with the design draft
P@niuguyThe solution looks perfect! I just have one small question for myself— I noticed you imported images from the public folder, but Vite recommends using src/assets as the base path for image imports. How would you adjust for that?
- @MomonkueiWhat are you most proud of, and what would you do differently next time?
nothing
What challenges did you encounter, and how did you overcome them?nothing
What specific areas of your project would you like help with?nothing
P@niuguyGood solution! just the feature cards overlap with each other a little bit on my desktop
- P@AnonymousCoder323P@niuguy
On desktop, it looks perfect. On mobile, the layout is good, but sometimes it doesn’t fully fit the screen, requiring horizontal scrolling. I’m not an expert in mobile layouts, especially with Tailwind. If you find a better solution later, please share it.
- @samuelgomez05P@niuguy
All is good to me. I like the way you organise the variables in the page, it looks clean. Thanks for sharing.
- @measmeas1P@niuguy
The styles on mobile version seems not correct
Here are some suggestions from cursor just FYI
- Font Import: The @import statement for fonts is quite complex and may lead to performance issues. It includes multiple font families with a range of weights and styles, which can increase loading time unnecessarily.
- Variable Usage: While CSS variables are used effectively, the naming convention could be improved for better readability. For example, --grey-700, --grey-800, and --grey-900 could be more descriptive.
- Container Width: The .container class has a fixed width of 18%, which may not be suitable for all screen sizes. A more responsive approach would be to use max-width or width in rem or px. Button Size: The .btn class has a fixed height of 40px, which may not accommodate different font sizes or padding well. Consider using padding instead of a fixed height for better flexibility. Color Contrast: The hover state for the button changes the text color to var(--grey-700), which may not provide sufficient contrast against the green background, potentially affecting accessibility.
- Lack of Comments: The CSS lacks comments, which can help in understanding the purpose of different styles, especially in larger stylesheets.
- Inconsistent Class Naming: The class names like .profile-img and .social-link do not follow a consistent naming convention with the original CSS, which may lead to confusion when integrating styles.
Marked as helpful - @220100dungvan
- @PaulgultiWhat challenges did you encounter, and how did you overcome them?
The parent div with the 100vh vh needs the property display flex inorder for the child div elements to arrange vertically.
P@niuguy(Below is generated by cursor and edited by myself)
- Semantic HTML Looking at the HTML structure:
<div class="body h-[100vh] flex bg-slate-300"> <div class="card w-[280px] h-[420px] m-auto bg-white rounded-2xl p-3"> <div class=""> <img class="rounded-lg" src="images/image-qr-code.png" alt="qr-code-image"> </div> <div class="p-4 mx-auto"> <h1 class="font-bold text-slate-900 text-center">Improve your front-end skills by building projects</h1> <p class="font-normal text-slate-500 text-[12px] text-center my-2">Scan the QR code to visit Frontend Mentor and take your coding skills to the next level</p> </div> </div> </div>
he solution uses basic div elements without semantic HTML tags. It could be improved by using:
- main for the main content wrapper
- article or section for the card component
- header for the title section
- footer for the descriptive text
- Accessibility
Several accessibility improvements could be made:
- Missing alt text is descriptive but could be more specific for the QR code
- No ARIA landmarks or roles are defined
- Color contrast appears good (dark text on white background)
- Missing language attributes for text content
- No skip links (though not critical for this simple layout) Suggested improvements:
<main class="h-[100vh] flex bg-slate-300"> <article class="card w-[280px] h-[420px] m-auto bg-white rounded-2xl p-3" role="article"> <div class="qr-container"> <img class="rounded-lg" src="images/image-qr-code.png" alt="QR code linking to Frontend Mentor website" /> </div> <header class="p-4 mx-auto"> <h1 class="font-bold text-slate-900 text-center"> Improve your front-end skills by building projects </h1> <p class="font-normal text-slate-500 text-[12px] text-center my-2"> Scan the QR code to visit Frontend Mentor and take your coding skills to the next level </p> </header> </article> </main>
- Responsive Design
The layout uses:
- Flexbox for centering
- Fixed width (w-[280px]) and height (h-[420px]) for the card
- Viewport height (h-[100vh]) for full-height layout While this works, it could be improved by:
- Using responsive units (rem/em) instead of fixed pixels
- Adding media queries for different screen sizes
- Considering minimum margins for very small screens
- Code Structure The code is: ✅ Relatively clean and readable ✅ Uses Tailwind CSS classes logically ✅ Maintains consistent indentation Areas for improvement:
- Separate components into reusable parts
- Use more semantic class names
- Consider extracting common styles into Tailwind components
- Design Fidelity
The implementation appears to be aligned with requirement on:
- Centered card layout
- Rounded corners
- Appropriate spacing
- Typography hierarchy
- Color scheme using slate colors
Marked as helpful