Skip to content
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

Checkbox Component #62

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

iamdarshshah
Copy link
Contributor

Based on: #12

  • Fix: Display checkbox component on components page

  • Added different checkbox component behaviours

  • Disabled Checkbox

  • Default Checked

  • Updated Documentation for Checkbox Component

@vercel
Copy link

vercel bot commented May 5, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sid/react-ui/6fk3urz1k
✅ Preview: https://react-ui-git-fork-iamdarshshah-checkbox-fix.sid.now.sh

@iamdarshshah
Copy link
Contributor Author

iamdarshshah commented May 7, 2020

@siddharthkp or @rubenmoya Could you please review this and let me know if any required changes need to be done.

`This also includes

  • Documentation for Radio Component.

  • Radio Component added

Thus, if this works fine we can close PR: #54 .`

Thanks

@rubenmoya
Copy link
Collaborator

Hello! Thank you for the contribution!

What scares me the most about the Checkbox and Radio components is the customisation, there has been a lot of times I've had to hide the input and use the label and :checked + label to achieve what the design team wanted.

Imagine we're asked to use this svg as the check mark:

<svg width="15" height="15" fill="none" xmlns="http://www.w3.org/2000/svg" style="display: block;"><path d="M11.467 3.72686C11.7559 3.91576 11.8369 4.30309 11.648 4.592L7.39803 11.092C7.29787 11.2452 7.1356 11.3468 6.95405 11.3699C6.77251 11.3931 6.58992 11.3355 6.4545 11.2124L3.7045 8.71243C3.44909 8.48024 3.43027 8.08496 3.66246 7.82954C3.89465 7.57413 4.28993 7.55531 4.54534 7.7875L6.75295 9.79442L10.6018 3.90793C10.7907 3.61903 11.178 3.53796 11.467 3.72686Z" fill="currentColor" fill-rule="evenodd" clip-rule="evenodd"></path></svg>

How could we achieve that? Is that something that React UI wants to support? Should we use a customized checkbox by default so it's easier to override styles?

Pinging @siddharthkp for input, since I don't know how we want to approach this

@iamdarshshah
Copy link
Contributor Author

iamdarshshah commented May 10, 2020

Yes, we can customize that as per the design flow. But before that, as you said we need to list down the things to include in checkbox component. This will help us understand what way should we proceed.

@siddharthkp it would be great if we have a list of things that needs to include in checkbox component for React UI.

@siddharthkp
Copy link
Owner

Heyo! Just wanted to say I have seen this PR but don't have any opinions yet. Will do my research and come back!

@siddharthkp siddharthkp changed the base branch from master to main June 27, 2020 08:53
@ksorv
Copy link

ksorv commented Jul 12, 2020

@rubenmoya indeed has a great point, Most of the time when working with UIs, It's exhausting to remove the default Checkbox/Radio styles and use something of your own.

@siddharthkp It'd would be to nice if we can add something like:

<Checkbox render={<Whatever />} {...props} />

Render being just a reference here, It can be custom Checkbox svg or something like that which will be used instead of the default one. Idk tho, is that good?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants