-
-
Notifications
You must be signed in to change notification settings - Fork 7
[DSRN] Added BadgeCount #472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
georgewrmarshall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Left one non-blocking suggestion otherwise LGTM 🚀
- Checked component against Figma designs ✅
- Checked stories and controls ✅
packages/design-system-react-native/src/components/BadgeCount/README.md
Outdated
Show resolved
Hide resolved
…README.md Co-authored-by: George Marshall <georgewrmarshall@gmail.com>
georgewrmarshall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Can we use the default Tailwind CSS classes for sizing where possible? This aligns directly with our spacing system, reinforcing consistency instead of relying on custom values. I believe this is generally better practice.
Can you point out which area is still not using this spacing system? |
georgewrmarshall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a suggestion to simplify the tests while maintaining coverage. We can likely apply this to most of these PRs but we can adjust the testing in a subsequent PR
| MAP_BADGECOUNT_SIZE_LINEHEIGHT[customSize], | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your intent with ensuring we have good testing coverage here but I think these tests seem overly complex and test implementation details that make them brittle. For tests we should be focusing on:
- Core functionality:
- count display
- overflow behavior (max prop)
- Prop behavior:
- textProps customization
- style prop
- size prop
- prop forwarding
georgewrmarshall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIght need to look at the spacing system for correct tailwind classnams I don't think it supports h-3.5
Description
This PR adds the
BadgeCountcomponent to the@metamask/design-system-react-nativepackageRelated issues
Fixes: #400
Manual testing steps
yarn storybook:iosfrom rootScreenshots/Recordings
Before
After
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-03-10.at.21.04.23.mp4
Pre-merge author checklist
Pre-merge reviewer checklist