Fixed 'ListItem' icons #473
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces updates to the frontend's icon system and styling for the Changes
Possibly Related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.jsx (1)
4-4: Yo dawg, let's clean up that import path!That import path's longer than mom's spaghetti recipe! Consider using path aliases to make it cleaner, eh?
-import { DuplicateIcon, EditIcon, TrashIcon } from '../../../../../../public/svg/svg'; +import { DuplicateIcon, EditIcon, TrashIcon } from '@svg/svg';
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/public/svg/svg.jsx(1 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.jsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (1)
frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.jsx (1)
33-39: Beauty implementation of those icons, eh!The new icons are lookin' sharp in those IconButtons. Matches the Figma designs perfectly!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/public/svg/svg.jsx (1)
29-30: Yo dawg, that stroke-width is making me nervous! 😰Let's keep it consistent with the other icons:
- stroke-width="1.66667" + stroke-width="1.5"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/public/svg/svg.jsx(1 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (2)
frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.css (1)
62-62: Yo, these changes are fresher than mom's spaghetti! 🍝Nice work using theme variables for both colour and font size. The changes align perfectly with our design system.
Also applies to: 67-68
frontend/public/svg/svg.jsx (1)
1-53: These icons are straight fire! 🔥Clean implementation with:
- Consistent dimensions (20x20)
- Theme variables for colors
- Well-structured SVG markup
How does 'utility-icons.jsx' sounds? |
Sounds good |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/public/svg/utility-icons.jsx (1)
19-35: Mom's spaghetti moment: Inconsistent stroke width!The TrashIcon's stroke-width (1.66667) is different from the other icons (1.5). Let's keep it consistent:
<path d="M13.3333 4.99996V4.33329C13.3333 3.39987 13.3333 2.93316 13.1517 2.57664C12.9919 2.26304 12.7369 2.00807 12.4233 1.84828C12.0668 1.66663 11.6001 1.66663 10.6667 1.66663H9.33333C8.39991 1.66663 7.9332 1.66663 7.57668 1.84828C7.26308 2.00807 7.00811 2.26304 6.84832 2.57664C6.66667 2.93316 6.66667 3.39987 6.66667 4.33329V4.99996M8.33333 9.58329V13.75M11.6667 9.58329V13.75M2.5 4.99996H17.5M15.8333 4.99996V14.3333C15.8333 15.7334 15.8333 16.4335 15.5608 16.9683C15.3212 17.4387 14.9387 17.8211 14.4683 18.0608C13.9335 18.3333 13.2335 18.3333 11.8333 18.3333H8.16667C6.76654 18.3333 6.06647 18.3333 5.53169 18.0608C5.06129 17.8211 4.67883 17.4387 4.43915 16.9683C4.16667 16.4335 4.16667 15.7334 4.16667 14.3333V4.99996" stroke="var(--border-error-solid)" - stroke-width="1.66667" + stroke-width="1.5"Also, don't forget to add accessibility attributes:
<svg + role="img" + aria-label="Delete item" width="20" height="20"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/public/svg/utility-icons.jsx(1 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/public/svg/utilityIcons.jsx (2)
1-53: Yo dawg, let's make these icons more accessible and DRY!The icons are looking fresh, but we could make them even better by:
- Adding ARIA attributes for accessibility
- Extracting common SVG props to reduce repetition
Here's a suggested implementation that's straight fire:
const IconWrapper = ({ children, title, ...props }) => ( <svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg" role="img" aria-hidden="false" aria-labelledby={title} {...props} > <title id={title}>{title}</title> {children} </svg> ); export const DuplicateIcon = () => ( <IconWrapper title="duplicate"> <path d="M4.16663 12.5001C3.39006 12.5001 3.00177 12.5001 2.69549 12.3732..." stroke="var(--second-text-color)" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round" /> </IconWrapper> );
19-35: Knees weak, arms heavy... this stroke-width is unsteady!The TrashIcon uses a stroke-width of 1.66667 while other icons use 1.5. Let's keep it consistent across all icons.
- stroke-width="1.66667" + stroke-width="1.5"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/public/svg/utilityIcons.jsx(1 hunks)frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/templates/GuideMainPageTemplate/GuideMainPageComponents/List/ListItem/ListItem.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (3)
frontend/public/svg/utilityIcons.jsx (3)
1-17: Mom's spaghetti approves this DuplicateIcon! 🍝The implementation is clean with appropriate stroke width and color variable usage.
37-53: There's no vomit on this sweater - EditIcon is ready! 🎯The implementation is consistent with the design system, using appropriate color variables and stroke width.
11-11: Verify these CSS variables are defined in the theme, yo!Let's make sure the color variables
--second-text-colorand--border-error-solidare properly defined in the theme.Also applies to: 29-29, 47-47
✅ Verification successful
Yo dawg, these CSS variables are straight fire! 🔥
Both
--second-text-colorand--border-error-solidare properly defined infrontend/src/styles/variables.css. We're good to go!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for color variable definitions rg -g '*.css' -g '*.scss' "second-text-color|border-error-solid"Length of output: 3043
|
Looks good! |
- Might need to change the name of 'utilityIcons.jsx' file to 'UtilityIcons.jsx' for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/public/svg/IconWrapper.jsx (1)
5-7: Yo dawg, these hardcoded dimensions are making me nervous!Consider making width and height configurable through props with default values of 20. This would make the wrapper more reusable for icons of different sizes.
- width="20" - height="20" + width={props.width || 20} + height={props.height || 20}frontend/public/svg/utilityIcons.jsx (1)
8-8: There's inconsistency on his sweater already! 🍝The stroke widths are inconsistent across icons:
- DuplicateIcon: 1.5
- TrashIcon: 1.66667
- EditIcon: 1.5
Consider standardizing to 1.5 for visual consistency.
- stroke-width="1.66667" + stroke-width="1.5"Also applies to: 20-20, 32-32
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/public/svg/IconWrapper.jsx(1 hunks)frontend/public/svg/utilityIcons.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (2)
frontend/public/svg/IconWrapper.jsx (1)
3-4: Mom's spaghetti approved! 🍝 Solid accessibility implementation!The use of
role="img"andaria-labelprops ensures proper screen reader support.frontend/public/svg/utilityIcons.jsx (1)
7-7: Straight fire! 🔥 Theme variables on point!Clean implementation using CSS variables for colors, making it easy to maintain consistent theming.
Also applies to: 19-19, 31-31
Describe your changes
Changed MUI icons on ListItem to the one on figma file.
Issue number
#472
Please ensure all items are checked off before requesting a review:
Preview