feat(apps/studio): Checkbox implementation for Storybook #688
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
@melihcanclk is attempting to deploy a commit to the kamp-us Team on Vercel. A member of the Team first needs to authorize it. |
| Use different settings for my mobile devices | ||
| </FormLabel> | ||
| <FormDescription> | ||
| You can manage your mobile notifications in the <Link href="/examples/forms">mobile settings</Link> page. |
There was a problem hiding this comment.
What was the purpose of using Link component here? If it was for routing, let's not use Link, if it was for an icon, lets rearrange it so it won't be pushed to a new line.
There was a problem hiding this comment.
In shadcn ui checkbox, there were some examples about checkbox and i tried to integrate those examples into our storybook but in form the link component was in use so i used it. It's my mistake that I didn't look how it should be look like in shadcn ui. I'll fix this now by removing Link component.
There was a problem hiding this comment.
no problem at all! ty so much for the effort! 🚀
| }, | ||
| } satisfies Story; | ||
|
|
||
| export const OnFocus = { |
There was a problem hiding this comment.
@melihcanclk Easiest would be creating a component inside this file and use state with proper onFocus handlers over there
There was a problem hiding this comment.
I tried to remove component inside OnFocus story and implement a component that makes handling with useState hook but realize that i have to give onFocus function outside of the component so to change the state inside onFocus function, i shouldn't make changes in Checkbox props, meaning it should take {...props} and with changing args to that component, we should do the change but this is my idea. If there is any way to change text inside the component by giving useState from args, I can implement it but this is what i did
I can commit this without the args at the down below in the story part or I can remove the whole onFocus part either because it really doesn't metter, why do i think in this way is that in Shadcn UI components' examples, there is no onFocus example. It has only With Text, Disabled and Form examples where all are implemented already, and if this is ok, i can do one extra thing that i did think that i could be doing but i didn't because of this same reason which is changing color of background and border of the checkbox over table down below.
There was a problem hiding this comment.
I made some comments about this and I think the way you handled in the new version looks good to me! After you make those changes, we can review it again and merge this :) Thank you so much for the hard work and thinking about additions to the regular use cases 🚀 💯
There was a problem hiding this comment.
I've tried to fix those and I'll be waiting for your feedback, thank you for your comments dear @cansirin 💜.
cansirin
requested review from
usirin
and removed request for
gurkanguray and
igdiaysu
| <div className="grid gap-1.5 leading-none"> | ||
| <Label | ||
| htmlFor="terms1" | ||
| className="text-sm font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70" |
There was a problem hiding this comment.
we should give a font color to this label, ideally text-primary.
| render: ({ ...props }) => <OnFocusTemplate {...props} />, | ||
| } satisfies Story; | ||
|
|
||
| const DescriptionTemplate = ({ ...props }) => { |
There was a problem hiding this comment.
we can rename this as DefaultTemplate
| })} | ||
| <div className="flex items-center space-x-2"> | ||
| <Checkbox id="checkbox" | ||
| onFocus={() => { |
There was a problem hiding this comment.
we should also use onBlur handler in order to handle focus loss.
| {...props} /> | ||
| <Label | ||
| htmlFor="checkbox" | ||
| className="text-sm font-medium leading-none peer-disabled:cursor-not-allowed peer-disabled:opacity-70" |
There was a problem hiding this comment.
we need font color here too, please use text-primary
Description
Checklist
cileknpx turbo runat the root of the repository, and build was successful.npm install --save-exact <package>so my package is pinned to a specific npm version. Leave empty if no package was installed. Leave empty if no package was installed with this PR.How were these changes tested?
Please describe the tests you did to test the changes you made. Please also specify your test configuration.