Select input component by mperrotti · Pull Request #1736 · primer/react

mperrotti · 2021-12-14T20:27:09Z

Adds a styled native <select> input component

Closeshttps://github.com/github/primer/issues/540

Screenshots

Kapture.2021-12-14.at.15.24.42.mp4

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2021-12-14T20:27:11Z

🦋 Changeset detected

Latest commit: fa45ce6

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

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

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 · 2021-12-14T20:30:06Z

size-limit report 📦

Path	Size
dist/browser.esm.js	60.42 KB (+0.8% 🔺)
dist/browser.umd.js	60.77 KB (+0.77% 🔺)

mperrotti · 2021-12-14T22:31:38Z

@@ -0,0 +1,100 @@
+---
+title: Select


I'm thinking maybe "SelectInput" is a better name. I'd love to hear what other reviewers think.

Yep! I think so too. It kinda cements it as a Form component and not any fancy menu/overlay interface

We only recently decided to avoid using *Input suffix on form components, so I'm in favour of keeping it as Select here. Reason: a) To match the underlying HTML and b) be consistent with other form components like Radio, Checkbox.

pksjce · 2021-12-15T00:22:18Z

There seems to be a clash here 😅

#1731

I'll close mine, as I'm yet to implement optgroups and review yours.

pksjce · 2021-12-15T01:06:57Z

+
+export default Object.assign(Select, {
+ Option,
+ Group


Can we name this OptGroup?

➕ . Can we call it OptionGroup to avoid the abbreviation?

I think if we're going to call the parent component Select, OptGroup might make more sense because it's closer to the HTML tag name: <optgroup>

pksjce · 2021-12-15T01:11:16Z

+ {...rest}
+ >
+ {placeholder ? (
+ <option value="" disabled={required} selected hidden={required}>


I think we need a storybook to show what this looks like?

You can see it by using the Controls. Do you think we need something more prominent?

pksjce · 2021-12-15T01:12:33Z

+ type: 'boolean'
+ }
+ },
+ contrast: {


This shouldn't be relevant for this component

Ok. I can remove it and we can always add it later if we decide we want parity with TextInput

pksjce · 2021-12-15T01:12:48Z

+ }
+ },
+ variant: {
+ name: 'Variant',


can we call this size?

Yes, absolutely. I was waiting for #1661 to merge which deprecates variant and adds size.

pksjce · 2021-12-15T01:15:36Z

Some comments

Should it show required * symbol?
All the dark mode colors seem broken?

Dark mode

Dark high contrast

Dark color blind

rezrah · 2021-12-15T14:00:24Z

+storybook: '/react/storybook?path=/story/forms-select--default'
+---
+
+import {ComponentChecklist} from '../src/component-checklist'


Don't think we need to import this anymore as it's global

That's what I thought too, but if I don't import it, it doesn't render.

Could your /docs node_modules need updating?

rezrah · 2021-12-15T14:13:29Z

+
+const StyledSelect = styled(TextInputWrapper)<SelectProps>`
+ appearance: none;
+ background-image: url(data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMTYiIGhlaWdodD0iMTYiIHZpZXdCb3g9IjAgMCAxNiAxNiIgZmlsbD0iIzU4NjA2OSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj48cGF0aCBkPSJNNC40MjcgOS40MjdsMy4zOTYgMy4zOTZhLjI1MS4yNTEgMCAwMC4zNTQgMGwzLjM5Ni0zLjM5NkEuMjUuMjUgMCAwMDExLjM5NiA5SDQuNjA0YS4yNS4yNSAwIDAwLS4xNzcuNDI3ek00LjQyMyA2LjQ3TDcuODIgMy4wNzJhLjI1LjI1IDAgMDEuMzU0IDBMMTEuNTcgNi40N2EuMjUuMjUgMCAwMS0uMTc3LjQyN0g0LjZhLjI1LjI1IDAgMDEtLjE3Ny0uNDI3eiIgLz48L3N2Zz4=);


Is there a way we can show the icon without using a base64 uri? It's quite expensive on the runtime and AFAIK a new pattern to PRC that could set a slippery precedent. Also, this image in its current form seems incompatible with other colour modes. Worth looking at pulling this in as a composable icon instead?

Those are both great points. I'll investigate alternatives.

rezrah · 2021-12-15T14:14:29Z

+ background-size: 16px;
+ padding-right: 20px;
+ cursor: pointer;
+


worth setting the background-color here too?

That should be handled in the TextInputWrapper component.

mperrotti · 2021-12-15T15:14:26Z

Should it show required * symbol?

That will be handled by the InputFieldComponent added in #1611

All the dark mode colors seem broken?

That's no good. I'll take a look.

Co-authored-by: Rez <rezrah@github.com>

…ect-input

rezrah · 2021-12-17T14:41:02Z

+
+expect.extend(toHaveNoViolations)
+
+describe('Radio', () => {


Should be Select here

rezrah · 2021-12-17T14:45:14Z

+
+ const select = getByLabelText('Choice')
+
+ expect(select.getAttribute('aria-disabled')).toBeTruthy()


❤️ . Personal preference, but@testing-library/jest-dom has some nice, more easily-readable matchers for this type of test. E.g.
Suggested change
expect(select.getAttribute('aria-disabled')).toBeTruthy()
expect(select).toHaveAttribute('aria-disabled')

…ect-input

rezrah

Looks good to go, nice one @mperrotti. Just a heads up, this won't appear on the primer.style/status page until you add comonentId to the front-matter. Might be worth sticking that into this PR or in a follow-up.

…ect-input

adds Select component and stories
520a056

mperrotti requested review from a team and siddharthkp December 14, 2021 20:27

mperrotti marked this pull request as draft December 14, 2021 20:27

mperrotti added 4 commits December 14, 2021 16:45

adds tests
917423c

styling fix for Firefox
ffa2183

adds docs
3bcebcb

adds changeset
0f77d9d

mperrotti marked this pull request as ready for review December 14, 2021 22:20

appease the linter
16b5f6a

mperrotti commented Dec 14, 2021
View reviewed changes

pksjce mentioned this pull request Dec 15, 2021
Select Input react component #1731
Closed
10 tasks

pksjce reviewed Dec 15, 2021
View reviewed changes

rezrah reviewed Dec 15, 2021
View reviewed changes

Comment threadsrc/Select.tsx Outdated

mperrottiand others added 8 commits December 15, 2021 10:17

Update src/Select.tsx
bc06aba
Co-authored-by: Rez <rezrah@github.com>

Merge branch 'main' into mp/select-input
7379950

Merge branch 'main' of github.com:primer/react into mp/select-input
9b81e50

addresses PR feedback
8fe1d20

Merge branch 'mp/select-input' of github.com:primer/react into mp/sel…
7329286
…ect-input

Merge branch 'main' into mp/select-input
5303d0d

Merge branch 'main' into mp/select-input
95c3be8

Merge branch 'main' into mp/select-input
ccc3d8c

rezrah reviewed Dec 17, 2021
View reviewed changes

mperrotti added 7 commits December 17, 2021 09:52

addresses more PR feedback
fde3e76

adds props table to select
88a65c9

Merge branch 'mp/select-input' of github.com:primer/react into mp/sel…
5ccc2f2
…ect-input

Merge branch 'main' into mp/select-input
5cc9867

fix deployment failure
6e4df1a

Merge branch 'mp/select-input' of github.com:primer/react into mp/sel…
6a04106
…ect-input

fixes docs build
9ff8321

rezrah approved these changes Dec 17, 2021
View reviewed changes

mperrotti added 4 commits December 17, 2021 13:10

change ternary to AND operator
a90a58d

adds comopnentId to docs frontmatter
c6eb710

Merge branch 'main' into mp/select-input
8b659c7

Merge branch 'main' into mp/select-input
96909d7

mperrotti temporarily deployed to visual-testing January 10, 2022 14:44 Inactive

Merge branch 'main' into mp/select-input
e7c5d91

mperrotti temporarily deployed to visual-testing January 10, 2022 19:46 Inactive

mperrotti added 3 commits January 10, 2022 15:48

Merge branch 'main' of github.com:primer/react into mp/select-input
ee76cb3

adds Select as a possible InputField child
4b84d8d

Merge branch 'mp/select-input' of github.com:primer/react into mp/sel…
003e353
…ect-input

mperrotti temporarily deployed to visual-testing January 10, 2022 21:02 Inactive

updates package-lock.json
86ab778

mperrotti temporarily deployed to visual-testing January 11, 2022 00:24 Inactive

Merge branch 'main' of github.com:primer/react into mp/select-input
77832b4

mperrotti temporarily deployed to visual-testing January 11, 2022 14:44 Inactive

docs fixes
fa45ce6

mperrotti temporarily deployed to visual-testing January 11, 2022 15:21 Inactive

mperrotti merged commit 82961d4 into mainJan 11, 2022

mperrotti deleted the mp/select-input branch January 11, 2022 15:46

primer-css mentioned this pull request Jan 11, 2022
Release Tracking #1760
Merged


		const select = getByLabelText('Choice')

		expect(select.getAttribute('aria-disabled')).toBeTruthy()

	expect(select.getAttribute('aria-disabled')).toBeTruthy()
	expect(select).toHaveAttribute('aria-disabled')


		expect.extend(toHaveNoViolations)

		describe('Radio', () => {

Uh oh!

Conversation

mperrotti commented Dec 14, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Merge checklist

Uh oh!

changeset-botBot commented Dec 14, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actionsBot commented Dec 14, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pksjce commented Dec 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pksjce commented Dec 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rezrahDec 15, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mperrotti commented Dec 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rezrah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mperrotti commented Dec 14, 2021•
edited
Loading

changeset-botBot commented Dec 14, 2021•
edited
Loading

github-actionsBot commented Dec 14, 2021•
edited
Loading

rezrahDec 15, 2021•
edited
Loading