Skip to content

Add Checkbox form component - #1606

Merged
rezrah merged 27 commits into
mainfrom
feature/add-native-checkbox-form-component
Nov 24, 2021
Merged

Add Checkbox form component#1606
rezrah merged 27 commits into
mainfrom
feature/add-native-checkbox-form-component

Conversation

@rezrah

@rezrahrezrah commented Nov 15, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Checkbox ✅

Adds a native Checkbox component that provides a react abstraction over a native HTML checkbox input

Adapted from the Primer design guidelines for checkboxes

Before:
<input type="checkbox" />

After:
<Checkbox />

Try it here

Docs: https://primer-components-eo45yjh4t-primer.vercel.app/react/Checkbox
Storybook: https://primer-components-eo45yjh4t-primer.vercel.app/react/storybook?path=/story/forms-checkbox--default

Screenshots

Screenshot 2021-11-19 at 15 22 01

Screenshot 2021-11-19 at 15 21 08

Screenshot 2021-11-19 at 15 21 44

Screenshot 2021-11-19 at 15 20 51

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Resolveshttps://github.com/github/primer/issues/467
Contributes towards: https://github.com/github/primer/issues/432

@changeset-bot

changeset-botBot commented Nov 15, 2021•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 3f1f8b8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/componentsMinor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actionsBot commented Nov 15, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js57.31 KB (+0.38% 🔺)
dist/browser.umd.js57.72 KB (+0.41% 🔺)

@rezrahrezrah changed the title Feature/add native checkbox form componentAdd Checkbox form componentNov 15, 2021
@mperrotti
mperrotti self-requested a review November 15, 2021 18:34
mperrotti
mperrotti reviewed Nov 16, 2021
View reviewed changes

@mperrottimperrotti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this is still in progress, I kept my feedback limited to the visual design and component API.

I'll review the code once we're further along.

Comment threadsrc/Checkbox.tsx Outdated
Comment threadsrc/Checkbox.tsx Outdated
Comment threadsrc/Checkbox.tsx Outdated
Comment threadsrc/Checkbox.tsx Outdated
Comment threadsrc/Checkbox.tsx Outdated
Comment threaddocs/content/Checkbox.md Outdated
Comment threadsrc/Checkbox.tsx Outdated
@siddharthkpsiddharthkp mentioned this pull request Nov 19, 2021
Merged
6 tasks
@rezrah
rezrah marked this pull request as ready for review November 19, 2021 17:45
@rezrah
rezrah requested a review from a teamNovember 19, 2021 17:45
mperrotti
mperrotti reviewed Nov 19, 2021
View reviewed changes
Comment threadsrc/Checkbox.tsx Outdated
Comment threadsrc/Checkbox.tsx
aria-disabled={disabled ? 'true' : 'false'}
ref={ref || checkboxRef}
checked={indeterminate ? false : checked}
aria-checked={indeterminate ? 'mixed' : checked ? 'true' : 'false'}

@mperrottimperrottiNov 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice

Comment threadsrc/Checkbox.tsx
Comment threadsrc/Checkbox.tsx
/**
* An accessible, native checkbox component
*/
const Checkbox = React.forwardRef<HTMLInputElement, CheckboxProps>(

@mperrottimperrottiNov 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need to use forwardRef?

@rezrahrezrahNov 22, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It's there to make the checkbox more flexible when used in uncontrolled mode.

Comment threadsrc/index.ts
export {default as UnderlineNav} from './UnderlineNav'
export type {UnderlineNavProps, UnderlineNavLinkProps} from './UnderlineNav'

export {default as Checkbox} from './Checkbox'

@mperrottimperrottiNov 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any ideas how we could guard against engineers rendering a <Checkbox /> without a corresponding <label />?

I have some ideas, but it would be much easier to show over Zoom instead of text and pseudocode. Let me know if you want to chat on Monday.

@mperrottimperrottiNov 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Chatted with @rezrah about this: we're just going to check for an associated label with a linter instead of doing anything in the runtime.

mperrotti
mperrotti reviewed Nov 19, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.md Outdated
colebemis
colebemis reviewed Nov 23, 2021
View reviewed changes
Comment threadsrc/Checkbox.tsx Outdated
colebemis
colebemis reviewed Nov 23, 2021
View reviewed changes
Comment threadsrc/Checkbox.tsx Outdated
colebemis
colebemis reviewed Nov 23, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.md Outdated
colebemis
colebemis reviewed Nov 23, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.md
colebemis
colebemis reviewed Nov 23, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.md Outdated
colebemis
colebemis reviewed Nov 23, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.md Outdated

import {ComponentChecklist} from '../src/component-checklist'

`Checkbox` is a form component that applies Primer design language to a native HTML checkbox input element.

@colebemiscolebemisNov 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we move this into the description frontmatter and reword it to be a little more actionable (see other component descriptions):

---
title: Checkbox
description: Use checkboxes to ...
---

@rezrahrezrahNov 23, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Moved and content updated 👍

colebemis
colebemis reviewed Nov 23, 2021
View reviewed changes
Comment threaddocs/content/Checkbox.md Outdated
colebemis
colebemis approved these changes Nov 23, 2021
View reviewed changes

@colebemiscolebemis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀

@rezrah
rezrah merged commit 985120a into mainNov 24, 2021
@rezrah
rezrah deleted the feature/add-native-checkbox-form-component branch November 24, 2021 09:52
@primer-cssprimer-css mentioned this pull request Nov 24, 2021
Merged
pksjce pushed a commit that referenced this pull request Dec 20, 2021
@rezrah@pksjce
Add Checkbox form component (#1606)
a516a92
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@mperrottimperrottimperrotti approved these changes
@colebemiscolebemiscolebemis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

minor releasenew featuresnew component

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rezrah@mperrotti@colebemis