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

chore: Add checkbox-as-button example #1002

Merged
merged 7 commits into from
Feb 18, 2022

Conversation

SCasarotto
Copy link
Contributor

@SCasarotto SCasarotto commented Feb 8, 2022

This update adds a as button example for the Checkbox component:

Screenshots
image
image

How to test?

Run yarn dev
Visit http://localhost:3000/examples/checkbox-as-button.

Does this PR introduce breaking changes?
Nope, just an example

PS: This is my first PR here so I am unable to update #939 and am looking for constructive feedback. I have some spare time and wanted to contribute where I can. If I get the flow for these examples I can probably tackle a bunch of them.

@vercel
Copy link

vercel bot commented Feb 8, 2022

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

reakit – ./

🔍 Inspect: https://vercel.com/ariakit/reakit/72HfEkMsJmCKRuyNQrCpP4zbiMiz
✅ Preview: Canceled

[Deployment for 8257cff canceled]

ariakit – ./

🔍 Inspect: https://vercel.com/ariakit/ariakit/DAV946Hwuexyo4N4qMe3Dvor1ZNu
✅ Preview: https://ariakit-git-fork-scasarotto-checkbox-as-button-ariakit.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 8, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #1002 (167422a) into main (119321d) will increase coverage by 1.33%.
The diff coverage is 100.00%.

❗ Current head 167422a differs from pull request most recent head 8257cff. Consider uploading reports for the commit 8257cff to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1002      +/-   ##
==========================================
+ Coverage   67.96%   69.29%   +1.33%     
==========================================
  Files         177      180       +3     
  Lines        4367     4410      +43     
  Branches     1195     1209      +14     
==========================================
+ Hits         2968     3056      +88     
+ Misses       1398     1353      -45     
  Partials        1        1              
Impacted Files Coverage Δ
...checkbox/__examples__/checkbox-as-button/index.tsx 100.00% <100.00%> (ø)
packages/ariakit/src/popover/popover-state.ts 73.23% <0.00%> (-14.70%) ⬇️
packages/ariakit/src/menu/menu-bar.ts 18.18% <0.00%> (-4.05%) ⬇️
packages/ariakit/src/portal/portal.tsx 57.00% <0.00%> (-0.58%) ⬇️
packages/ariakit/src/dialog/dialog.tsx 87.71% <0.00%> (-0.45%) ⬇️
packages/ariakit/src/disclosure/disclosure.ts 100.00% <0.00%> (ø)
packages/ariakit/src/composite/composite-hover.ts 82.14% <0.00%> (ø)
packages/ariakit/src/hovercard/__debug-polygon.ts 0.00% <0.00%> (ø)
...t/src/focus-trap/__examples__/focus-trap/index.tsx 100.00% <0.00%> (ø)
...kages/ariakit/src/menu/__examples__/menu/index.tsx 100.00% <0.00%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 119321d...8257cff. Read the comment docs.

.vscode/settings.json Outdated Show resolved Hide resolved
@diegohaz
Copy link
Member

Thank you @SCasarotto!

I think this is actually a good example of checkbox-visually-hidden, checkbox-custom, or something similar.

A checkbox-button example should look more like a button than a checkbox. Since this is just a custom checkbox, we should render a visually hidden checkbox so we support native browser features (form submission, autofill etc.).

I'm thinking about something like this:

const checkbox = useCheckboxState();
const [focusVisible, setFocusVisible] = useState(false);
return (
  <label className="label">
    <VisuallyHidden>
      <Checkbox
        state={checkbox}
        onFocusVisible={() => setFocusVisible(true)}
        onBlur={() => setFocusVisible(false)}
      />
    </VisuallyHidden>
    <div
      className="checkbox"
      data-focus-visible={focusVisible ? "" : undefined}
      data-checked={checkbox.value}
    />
    I have read and agree to the terms and conditions
  </label>
);

@diegohaz diegohaz changed the base branch from v2 to master February 10, 2022 22:46
@SCasarotto
Copy link
Contributor Author

@diegohaz Thanks for the feedback. 🙏 I will probably migrate this over to fill the Checkbox styled using VisuallyHidden without useCheckboxState example and make these corrections.

@saideepesh000 saideepesh000 mentioned this pull request Feb 16, 2022
69 tasks
@SCasarotto
Copy link
Contributor Author

@diegohaz I figured I would keep this PR for the as-button example and open another one with the custom checkbox stuff. So chasing the as-button example I ran into some questions. In your example above, you aren't using the as="button" prop. I totally get that we want to have the native input on the page for all the native form functionality but I also assume we don't want to use <Checkbox/> twice (1 hidden and 1 as button). Is that right? Or asked a different way, with form inputs, because of all the responsibilities they have, when is it right or wrong to use the as={}?

… space interactions, removed the user of tailwinds content
@diegohaz
Copy link
Member

@diegohaz I figured I would keep this PR for the as-button example and open another one with the custom checkbox stuff. So chasing the as-button example I ran into some questions. In your example above, you aren't using the as="button" prop. I totally get that we want to have the native input on the page for all the native form functionality but I also assume we don't want to use <Checkbox/> twice (1 hidden and 1 as button). Is that right? Or asked a different way, with form inputs, because of all the responsibilities they have, when is it right or wrong to use the as={}?

We should always recommend a native checkbox (even if it's visually hidden) if the element is used within a form or is styled similarly to a native checkbox (which is the case here). A checkbox as a button shouldn't look like a checkbox. It could be a button with a checkmark, a menu item checkbox, or something else.

If we're already rendering a visually hidden native checkbox, we don't need another checkbox. In my example, the native checkbox will receive focus and the <div className="checkbox"> element is there just to display the custom styles.

@SCasarotto
Copy link
Contributor Author

Updated screenshots:
image
image
image
image

@SCasarotto
Copy link
Contributor Author

I think I updated all of those things.

Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you @SCasarotto! ❤️

@diegohaz diegohaz changed the title chore: Add Checkbox as button example chore: Add checkbox-as-button example Feb 18, 2022
@diegohaz diegohaz merged commit 5352a6a into ariakit:main Feb 18, 2022
@ariakit-bot
Copy link

Thanks a lot for contributing!

Based on our community guidelines, every person who has a PR of any kind merged is offered an invitation to the Reakit organization.

Should you accept, you'll get write access to the main repository and a fancy Reakit logo on your GitHub profile. You'll be able to label and close issues, create new branches etc. Make sure to read our contribution and community guidelines, which explains all of this in more detail.

If you have any questions, let me know!

@SCasarotto SCasarotto deleted the checkbox-as-button branch February 18, 2022 03:38
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.

3 participants