-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add Checkbox Component with Storybook Integration and Tests #19
Add Checkbox Component with Storybook Integration and Tests #19
Conversation
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.
Thank you for contributing! There are some slight adjustments to be made. Please let me know if any questions!
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.
./components/Checkbox
needs to be added here. It enables consumers of the package to import Checkbox
directly:
import { Checkbox } from '@stanfordspezi/spezi-web-design-system/components/Checkbox'
const newCheckedValue = false | ||
expect(onCheckedChange).toHaveBeenCalledWith(newCheckedValue) |
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.
It seems like newCheckedValue
variable is redundant. expect(onCheckedChange).toHaveBeenCalledWith
already tells us we're asserting on new value of checked.
const newCheckedValue = false | |
expect(onCheckedChange).toHaveBeenCalledWith(newCheckedValue) | |
expect(onCheckedChange).toHaveBeenCalledWith(false) |
import { cn } from '../../utils/className' | ||
import { CheckIcon } from 'lucide-react' | ||
|
||
export const Checkbox = forwardRef< |
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.
It needs displayName
property. There is ESLint rule to enforce that. Did it fire in your IDE?
>(({ className, ...props }, ref) => ( | ||
<CheckboxPrimitives.Root | ||
className={cn( | ||
'hover:bg-violet3 flex size-[25px] appearance-none items-center justify-center rounded bg-white shadow-[0_0_0_2px_black] outline-none', |
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 have not run this yet, but it seems like we need a bit more work here.
Components have to rely on the standard Tailwind's design tokens and design system custom color tokens. This enables consumers to modify the theme.
Focus states have to be properly handled. focus-ring
className should do the trick most likely.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
+ Coverage 81.07% 81.17% +0.10%
==========================================
Files 125 127 +2
Lines 2435 2453 +18
Branches 276 277 +1
==========================================
+ Hits 1974 1991 +17
- Misses 440 441 +1
Partials 21 21
Continue to review full report in Codecov by Sentry.
|
Closing this in favor of #36 due to inactivity. Thank you for working on this! |
Add Checkbox Component with Storybook Integration and Tests
♻️ Current situation & Problem
closes #15
Changes Included
Storybook Integration (Added Storybook stories for the Checkbox component)
Unit Tests
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: