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

fix(combobox): add combobox pattern #3894

Merged
merged 23 commits into from
Feb 2, 2024
Merged

fix(combobox): add combobox pattern #3894

merged 23 commits into from
Feb 2, 2024

Conversation

Westbrook
Copy link
Contributor

@Westbrook Westbrook commented Dec 19, 2023

Description

Add a Combobox pattern to the library:

<sp-combobox label="Declarative combobox">
    <sp-menu-item value="0">Declare this</sp-menu-item>
    <sp-menu-item value="1">Declare that</sp-menu-item>
    <sp-menu-item value="2">Declare the other</sp-menu-item>
</sp-combobox>

<sp-combobox
  label="Imparative combobox"
  options='[
    {"value": "0", "itemText": "Imperative this"},
    {"value": "1", "itemText": "Imperative that"},
    {"value": "2", "itemText": "Imperative the other"}
  ]'
>
</sp-combobox>

Visit the docs for more information.

Related issue(s)

How has this been tested?

  • Test case 1
    1. Go here
    2. Test an autocomplete="none" Combobox
  • Test case 2
    1. Go here
    2. Test an autocomplete="list" Combobox

Screenshots (if appropriate)

image

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Copy link

Tachometer results

Currently, no packages are changed by this PR...

@najikahalsema najikahalsema linked an issue Dec 19, 2023 that may be closed by this pull request
Comment on lines 43 to 46
export type ComboboxOption = {
id: string;
value: string;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question that came up for me as I work on the benchmark test: do we want to expand this API to also to accept an array of strings that could get converted to ComboboxOptions? The id could match the value, or we could do some fancy RegEx or whatever to convert the value to something more multipurpose.

The reason I ask is because I have an String array of a list of countries, and it's annoying for me to have to convert them into an object so I can fit it into this API. It'd be nice if I could just have the combobox do the work of converting it for me. I'm also thinking about that in case we end up using the select-only combobox as a basis for the lazy picker or something... What do you think, @Westbrook?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, we can do this with a map and have all the values in the lightDOM, but I wouldn't want that to be the only option.

        <sp-combobox>
            ${countryList.map(
                (value, index) => html`
                    <sp-menu-item id=${index} value=${value}>
                        ${option.value}
                    </sp-menu-item>
                `
            )}
        </sp-combobox>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great question for potential consumers of the element. I'm not opposed to it so much as I would be surprised to see someone with a functional data set that didn't already have not a unique identifier AND a user readable value.

In that case that there were, conditioning data to support this is also not too crazy:

<sp-combobox
    .options=${countryList.map((value, index) => ({ value, index }))}
></sp-combobox>

This repeats the data processing more than would be appropriate, so you would probably cache a conditioned array in the parent context. That's not so different than us doing it as well, so maybe we would just do it by default.

The question will always go back to whether it actually makes sense to for consumers to actually reference values by their visible value vs always having a unique ID for them.

@Westbrook Westbrook force-pushed the combobox branch 3 times, most recently from 56d8884 to 1477765 Compare January 9, 2024 22:54
@Westbrook Westbrook force-pushed the combobox branch 3 times, most recently from 14aac36 to 40cd849 Compare January 18, 2024 19:18
@Westbrook Westbrook force-pushed the combobox branch 4 times, most recently from 09cc8e7 to d34ecd4 Compare January 25, 2024 17:36
Westbrook and others added 16 commits January 31, 2024 10:46
* feat(combobox): wip

* chore: update sizes and stories

* chore: add isoverlayopen decorator to stories

---------

Co-authored-by: Westbrook Johnson <[email protected]>
Co-authored-by: Najika Yoo <[email protected]>
* chore: add benchmark test for lightdom combobox

* chore: add object version of benchmark test

* chore: rename files

---------

Co-authored-by: Najika Yoo <[email protected]>
…sited in future work (#3919)

* test(combobox): get more tests passing and skip tests that will be visited in future work

* ci: update golden images cache

* test(combobox): ignore Combobox Item code
* chore(combobox): cleanup unused code

* ci: update golden images cache
* fix(combobox): add support for external tooltip elements

* chore(combobox): remove unused code paths

* ci: update golden images cache

* docs(combobox): include slot present in API docs
* chore: add labels to combobox input

* chore: get tests passing

* test(combobox): get a11y tests passing

* chore: remove unused positionlistbox method

* test: get tests passing, change spelling of activeDescendant

* chore: missed some descendents

* chore: add help text demo and test

* ci: update hash

* chore: address review comments

* chore: abstract shared data to index files

* test(combobox): update tests and stories to use legible data

* ci: update hash

* chore: label menu and rename stories

* ci: update hash

---------

Co-authored-by: Najika Yoo <[email protected]>
* chore: add tooltip to ariadescribedby

* test(combobox): add a11y test for tooltip

---------

Co-authored-by: Najika Yoo <[email protected]>
* chore(combobox): clean up property availability and type

* refactor(combobox): update ComboboxOption type

* ci: update golden images cache

* refactor(combobox): simplify typing and correct query location when moving items into viewport

* docs: use human useful content in stories

* ci: update golden images cache
@Westbrook Westbrook force-pushed the combobox branch 2 times, most recently from 6a5c85c to 86d27f6 Compare January 31, 2024 16:18
hunterloftis and others added 2 commits February 1, 2024 09:54
…3988)

* docs(combobox): add story demonstrating controlled-component usage

* Update packages/combobox/stories/combobox.stories.ts

Co-authored-by: Westbrook Johnson <[email protected]>

---------

Co-authored-by: Westbrook Johnson <[email protected]>
* docs(textfield): expand on attribute/property descriptions

* fix(combobox): add support for "readonly" and "disabled"

* ci: update golden images cache

* fix(textfield): prevent outline on :focus-visible elements that are disabled
@Westbrook Westbrook requested a review from a team February 1, 2024 18:14
@Westbrook Westbrook changed the title feat(combobox): begin working branch for combobox additions fix(combobox): add combobox pattern Feb 1, 2024
@Westbrook Westbrook marked this pull request as ready for review February 1, 2024 18:15
@Westbrook Westbrook marked this pull request as draft February 1, 2024 20:11
@Westbrook Westbrook marked this pull request as ready for review February 2, 2024 14:45
@hunterloftis hunterloftis self-assigned this Feb 2, 2024
@hunterloftis
Copy link
Contributor

In earlier testing it appeared case-insensitive, but now it's case-sensitive:

image

vs

image

I'm not sure what the right default is between these two, just wanted to point out that change.

@Westbrook Westbrook merged commit 47d7d71 into main Feb 2, 2024
47 checks passed
@Westbrook Westbrook deleted the combobox branch February 2, 2024 19:29
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.

Add Spectrum Combobox Pattern
3 participants