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

Combobox explainer initial anatomy proposal #928

Merged
merged 7 commits into from
Nov 7, 2023
Merged

Combobox explainer initial anatomy proposal #928

merged 7 commits into from
Nov 7, 2023

Conversation

sudheer-gowrigari
Copy link
Collaborator

@sudheer-gowrigari sudheer-gowrigari commented Nov 3, 2023

From issue: #924

Screen.Recording.2023-11-03.at.4.06.14.PM.mov
Screenshot 2023-11-03 at 7 30 16 PM


## Research

To keep this doc focused on the initial recommended approach, we've separated the research for parts and examples into another document that can be [found here](https://github.com/sudheer-gowrigari/explainers/blob/main/combobox-research.md)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will be moving this research to open-ui in next PR

Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

Please open the following "non-blocking" issues for this PR:

  • combobx name
  • search attribute and values naming
  • filter name

Great PR. Certain changes I think are fine to move to an issue for discussion and shouldn't block the PR landing.


A good analogy for what this document will result in looking like is the [Tabs research document.](https://open-ui.org/components/tabs.research.parts/)

## Recommended anatomy for version 1
Copy link
Member

Choose a reason for hiding this comment

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

Let's change this simply to "Anatomy". This is meant to graduate as an element and this will be our recommendation so this is implied. Additionally, any future version (eg: supporting multiple) will outline its variation so denoting that there may need to be future versions isn't necessary.


```
<combobox>
<input type=selectlist>
Copy link
Member

Choose a reason for hiding this comment

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

Currently popovertarget (which this effectively uses under the hood) only supports buttons. We resolved in #839 that this should be extended to anchors as well. @lukewarlow raised a few weeks back that input should be added. This is a compelling reason for input to be added to the list.

Here is a jsbin that shows what is currently supported vs what you're proposing: https://jsbin.com/cedepubepe/edit?html,css,output

I recommend adding a "Question" (see class here ) that this anatomy would require input to be supported by invokertarget.

Copy link
Collaborator

@lukewarlow lukewarlow Nov 4, 2023

Choose a reason for hiding this comment

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

Fwiw combobox was exactly the usecase where I thought invoke (or I think probably actually interest) target would be useful on an input.

Having said that I'm not convinced input is the right call here (discuss separately of course not wanting to block this) due to its parser limitations that I don't forsee us getting past.

```


**Pros:**
Copy link
Member

Choose a reason for hiding this comment

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

Remove the pros and cons list

**Cons:**
* Introducing new element, `<combobox>`

### Anatomy of `<combobox>`:
Copy link
Member

Choose a reason for hiding this comment

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

Let's not duplicate the anatomy, put only one in and then define it.

- Attributes:
- **search**: Indicates the combobox will actively fetch results based on user input.
- **filter**: Indicates the combobox will narrow down visible options based on user input.
- **multiple**: Allows multiple options to be selected.
Copy link
Member

Choose a reason for hiding this comment

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

Since we won't be defining this in the current explainer, please denote in parenthesis that this will be defined in a future version in alignment with selectlist

* Contains
* ..

The current behavior of `<selectlist>` is the _startswith_ value
Copy link
Member

Choose a reason for hiding this comment

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

I believe the purpose of showing this is to recommend that startswith is that it should be the default value? You should denote this in some manner in the values

* pattern
* startswith
* Contains
* ..
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this. If we want to add more values later we can always do so

</listbox>
</combobox>
```
<Image
Copy link
Member

Choose a reason for hiding this comment

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

I like the example, but we also need to have ordered list algorithms that define how each value will function. Don't get too hung up on this but do lean on prior definitions with the WHATHWG HTML or TC39 specifications rather than re-inventing the wheel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added defs and examples. Also updated to includes from contains . as per TC39 includes




### Introduction of `multiple` attribute
Copy link
Member

Choose a reason for hiding this comment

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

Please be explicit that this will be defined in a later version in coordination with selectlist.


### Keyboard Behavior

TBD : [https://www.w3.org/WAI/ARIA/apg/patterns/combobox/](https://www.w3.org/WAI/ARIA/apg/patterns/combobox/)
Copy link
Member

Choose a reason for hiding this comment

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

File an issue and link to it here

@sudheer-gowrigari
Copy link
Collaborator Author

Please open the following "non-blocking" issues for this PR:

  • combobx name
  • search attribute and values naming
  • filter name

Great PR. Certain changes I think are fine to move to an issue for discussion and shouldn't block the PR landing.

Thanks for the review Greg. Created following issues and also linked them in the explainer doc.

  1. Naming combobox Element #930
  2. Combobox search attribute and its values #931
  3. Combobox filter attribute #932

@gregwhitworth gregwhitworth merged commit 29f8c23 into openui:main Nov 7, 2023
3 checks passed
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.

3 participants