-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add multiselect component #219
Conversation
🦋 Changeset detectedLatest commit: 73ed7de The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
11ae9bf
to
f89df01
Compare
Preview URLsEnv: preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This essentially matches the autocomplete demo. Almost copy/paste, with a few modifications.
@onChange={{this.onChange}} | ||
@options={{this.options}} | ||
@contentClass='z-10' | ||
@removeButtonLabelFunction={{this.formatRemoveButtonAriaLabelStringObject}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More about this below!
packages/ember-toucan-core/src/-private/components/form/controls/multiselect/option.ts
Outdated
Show resolved
Hide resolved
packages/ember-toucan-core/src/-private/components/form/controls/multiselect/option.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this should resemble the autocomplete component. The difference (mostly) is how the selected items are rendered as chips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A big chunk of this was taken from autocomplete as well. There are a few changes sprinkled throughout though. I tried to comment all of the actions and logic as best I could. Please let me know if I missed anything or if I can explain things better!
…ls/multiselect/option.ts
…ls/multiselect/option.ts
…ls/multiselect/option.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed just the tests for now, left a few comments - thank you for writing these exhaustive tests!
|
||
assert.dom('[data-multiselect-selected-option]').exists({ count: 2 }); | ||
|
||
let chips = document.querySelectorAll('[data-multiselect-selected-option]'); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
||
await click('[data-multiselect]'); | ||
|
||
// Since `@selected=["blue"]`, we expect it to be selected |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
['b', 'c'], | ||
'Expected "a" to be removed on change as it was re-selected' | ||
); | ||
assert.step('handleChange'); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
assert.dom('[role="listbox"]').exists(); | ||
|
||
// Now blur the elment | ||
await click('[data-input]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, doesn't this focus the <input />
element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can add a comment here, but we are blurring the input on https://github.com/CrowdStrike/ember-toucan-core/pull/219/files/16695506e740b9b0240a57b5faf3f3f14193e104#diff-136cfdc764a84882ddf48cd853edc7085a558470ed9eb3d8b4d33de1adfa8795R958 (the non-multiselect-input), not the multiselect input!
assert.dom('[role="option"]').exists({ count: 2 }); | ||
// We can also verify our data attributes are spread to each option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Very helpful
assert('The `:remove` block is required.', removeBlockExists); | ||
|
||
if (!removeBlockExists) { | ||
return false; | ||
} | ||
|
||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets cleaned up in the next commit
{{yield | ||
(hash | ||
option=option | ||
Remove=(component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whose bright idea was it to name this "Remove"? 😆
assert.dom('[data-multiselect]').hasAttribute('aria-expanded'); | ||
}); | ||
|
||
test('it sets `aria-controls`', async function (assert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see this test being slightly more valuable if its value were also tested for and not just its existence. Though I could also see an argument for removing this test given its minimal value.
aria-activedescendant
strikes me as perhaps a better test candidate.
{{! template-lint-disable no-pointer-down-event-binding }} | ||
<button | ||
aria-label={{@label}} | ||
class="focusable reset-styles p-1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind dumping a copy of our reset-styles
TODO here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch, thank you! Added at 73ed7de
Going to go ahead and merge as to unblock other downstream work, but please feel free to keep commenting and reviewing! I'll also have Komal take a look over everything in the middle of next week. |
🚀 Description
This PR adds the Multiselect control component.
What's next in downstream PRs:
toucan-form
🔬 How to Test
📸 Images/Videos of Functionality
Screen.Recording.2023-07-19.at.2.05.02.PM.mov