Create Selectable component #660
Conversation
lazytype
force-pushed
the
PD-271-Select
branch
3 times, most recently
from
542a41e to
9895ba2
Compare
|
Size Change: 0 B Total Size: 320 kB ℹ️ View Unchanged
|
lazytype
force-pushed
the
PD-271-Select
branch
from
9895ba2 to
2f2257c
Compare
lazytype
force-pushed
the
PD-271-Select
branch
from
2f2257c to
d542ff4
Compare
packages/select/src/ListMenu.tsx
Outdated
| background-color: ${colorSet.option.background.base}; | ||
| box-shadow: 0 3px 7px 0 ${colorSet.menu.shadow}; | ||
|
|
||
| @media only screen and (max-width: ${breakpoints.Desktop}px) { |
There was a problem hiding this comment.
we use facepaint to help with media queries. not sure if necessary here, just a thought
packages/select/src/MenuButton.tsx
Outdated
| const sizeSet = sizeSets[size]; | ||
|
|
||
| const baseBoxShadow = `inset 0 -1px 0 0 ${colorSet.shadow.base}`; | ||
| let boxShadowExpanded = `0 0 0 3px ${colorSet.shadow.expanded}`; |
There was a problem hiding this comment.
is there a reason not to turn this into a ternary?
There was a problem hiding this comment.
the boxShadowExpanded definition below uses the value of this definition
|
Very very excited to have this in the repo |
lazytype
force-pushed
the
PD-271-Select
branch
from
21e337f to
fbeaf6a
Compare
…o PD-271-Select
packages/select/src/ListMenu.tsx
Outdated
| const onKeyDown = useCallback( | ||
| (event: React.KeyboardEvent) => { | ||
| // No support for modifiers yet | ||
| /* istanbul ignore if */ |
There was a problem hiding this comment.
do we need these comments in production too?
There was a problem hiding this comment.
it only affects test coverage reporting. it should get stripped out when bundled
There was a problem hiding this comment.
Just took a first look at this PR, and noticed a few things from an interaction perspective.
- The dropdown doesn't transition in any way. Is there a reason we don't use Portal for this component? It has transitions built in.
- The gradient, and border for the trigger don't transition between states. We should transition those like we do our button (if not using the button itself directly).
- The interaction ring's background color doesn't appear to transition between background colors.
- In general, we should transition between all states (selected, hover, focus, open).
- It looks like the line-height of wrapping options is a bit too tight. Is that up to spec?
- I think @audroue has some designs ready for an updated selected state for the component. That would be great to include.
|
|
||
| export const sizeSets: Record<Size, SizeSet> = { | ||
| [Size.XSmall]: { | ||
| height: 22, |
There was a problem hiding this comment.
What's the benefit to this versus simply writing out the CSS declarations here?
packages/select/README.md
Outdated
| | `description` | `string` | Text that gives more detail about the requirements for the input. | | | ||
| | `placeholder` | `string` | The placeholder text shown in the input element when an option is not selected. | `'Select'` | | ||
| | `disabled` | `boolean` | Disables the component from being edited. | `false` | | ||
| | `value` | `string` | Sets the `<Option />` that will appear selected and makes the component a controlled component. | `''` | |
There was a problem hiding this comment.
Should we consider allowing ReactElements to be passed here as well? A use case I'm thinking about is when we have Icons that would be part of Option value. Let me know what you think cc @DesignerDave
There was a problem hiding this comment.
Agreed, just because weird use-cases do come up, and it's low effort to support the flexibility.
There was a problem hiding this comment.
I wouldn't consider it low-effort. It would require being able to parse out the text content of arbitrary JSX and even further work if we would want to be able to differentiate between <><XIcon/> Icon</> and <><PlusIcon/> Icon</>. I wouldn't want to implement this unless it was specifically asked for, "YAGNI"
There was a problem hiding this comment.
You're right 🤦 I was thinking this was in reference to the display value for an option for some reason.
There was a problem hiding this comment.
Hey Michael! This is looking close.
There's a few design-related issues to note that I can see:
- The padding to the left of the text in the select input toggle should be aligned with the padding of the labels in the dropdown. The caret on the right should also reflect this spacing.
- The hover state for select options should transition.
- Clicking the labels for option groups closes the dropdown. I'd confirm with Rob what the behavior should be here, but this feels like it would be annoying if you misclick when selecting an option.
- The dropdown should animate its position as it opens instead of only fading in (though we should keep the opacity transition as well). Adding a
translate3d(0, -8px, 0)to the inactive state, andtranslate3d(0, 0, 0)to the active one, and transitioning the value should improve things!
We might even want to go a little further and mimic our popover component:
# inactive
transition: opacity 150ms ease-in-out, transform 150ms ease-in-out;
opacity: 0;
transform: translate3d(0, -8px, 0) scale(0.95);
transform-origin: center top;
# active
opacity: 1;
transform: translate3d(0, -8px, 0) scale(0.95);…o PD-271-Select
…o PD-271-Select
packages/select/README.md
Outdated
| | `description` | `string` | Text that gives more detail about the requirements for the input. | | | ||
| | `placeholder` | `string` | The placeholder text shown in the input element when an option is not selected. | `'Select'` | | ||
| | `disabled` | `boolean` | Disables the component from being edited. | `false` | | ||
| | `value` | `string` | Sets the `<Option />` that will appear selected and makes the component a controlled component. | `''` | |
There was a problem hiding this comment.
I wouldn't consider it low-effort. It would require being able to parse out the text content of arbitrary JSX and even further work if we would want to be able to differentiate between <><XIcon/> Icon</> and <><PlusIcon/> Icon</>. I wouldn't want to implement this unless it was specifically asked for, "YAGNI"
That's fine! It'll look better visually overall, and align better with the specs. |
There was a problem hiding this comment.
Just want to note that I noticed that the combobox aria role is incorrectly applied here (as raised by both the accessibility knob and axe dev tools). I don't want to spend time trying to figure out on our own how to make this more accessible, as we're getting the results of the audit back on Wednesday, and I think their guidance will be very instructive in terms of next steps.
In the meantime, happy to get this in (maybe as a beta, since we know there will be some changes coming soon? cc: @DesignerDave )! Very excited to have this component in the repo.
I've brought this up in a previous meeting but I can document it here for posterity. I've implemented combobox in conformance with the WAI-ARIA 1.2 spec https://www.w3.org/TR/wai-aria-1.2/#combobox. The accessibility guidance provided by storybook and axe is still based on WAI-ARIA 1.1 https://www.w3.org/TR/wai-aria-1.1/#combobox |
|
dequelabs/axe-core#2505 is the tracking issue for Deque to update axe |
SGTM |
* Create Selectable component * PR feedback and adjustments * More code reuse * prettier * updated design * update readme * use button as input * Delegate focus to button * cleanup * use updated button * revert eslintignore * Make name optional * PR feedback * fix readme * prettier * supply darkMode prop to Buton * fix scroll * prettier * Give option group aria label and hide caret icon Co-authored-by: Michael Mitchell <[email protected]>
✍️ Proposed changes
🎟 Jira ticket: Select React Component
🛠 Types of changes
✅ Checklist
yarn changesetand documented my changes💬 Further comments