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 hooks docs #1276

Merged
merged 21 commits into from
Feb 11, 2021
Merged

Combobox hooks docs #1276

merged 21 commits into from
Feb 11, 2021

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Nov 16, 2020

Closes #1257

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@LFDanLu LFDanLu changed the title (WIP) Docs combobox hooks (WIP) Combobox hooks docs Nov 16, 2020
@LFDanLu LFDanLu changed the base branch from main to docs_combobox November 16, 2020 23:28
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Base automatically changed from docs_combobox to release-updates November 25, 2020 22:44
Base automatically changed from release-updates to main November 26, 2020 00:06
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@LFDanLu LFDanLu changed the title (WIP) Combobox hooks docs Combobox hooks docs Jan 5, 2021
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

shouldCloseOnBlur?: boolean
}

/**
* Provides state management for a ComboBox component. Handles building a collection
* of items from props and manages the option selection state of the ComboBox. Also tracks the input value,
Copy link
Member

Choose a reason for hiding this comment

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

For some reason this Also bugs me when I read it. Maybe In addition... or Additional functionality includes...

<InterfaceType properties={docs.links[docs.exports.useComboBox.return.id].properties} />
</TypeContext.Provider>

State is managed by the <TypeLink links={statelyDocs.links} type={statelyDocs.exports.useComboBoxState} /> hook from `@react-stately/select`.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a TypeLink instead directly to the stately docs page?

Copy link
Member Author

Choose a reason for hiding this comment

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

Follows the pattern set up by previous stately doc pages. The stately pages are typically pretty bare, so having this TypeLink is sufficient since it shows the stately hook's api.

);
}

<ComboBox label="Favorite Animal" defaultSelectedKey="red panda">
Copy link
Member

Choose a reason for hiding this comment

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

Should this example have an empty dropdown, seen as a line in the image, when nothing matches?
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I can't seem to get that to happen. What browser/repro steps?

Copy link
Member

Choose a reason for hiding this comment

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

Chrome (that is a version behind) and attaching video.

Screen.Recording.2021-01-14.at.3.19.05.PM.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh, I see, problem with the controlled story, lemme see if I can change something

Copy link
Member Author

Choose a reason for hiding this comment

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

kk, I pushed up a fix, good catch. I forgot to add items to the field state since that was controlled as well and useComboBoxState doesn't close the menu if the collection is empty and items/open state is controlled, that is up to the user event handlers

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should not have a default selected key? It's weird to click the button and only see one option...

///- end collapse -///
// Using the same ComboBox component code from the first example...

<ComboBox label="Favorite Animal" menuTrigger="focus">
Copy link
Member

Choose a reason for hiding this comment

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

Should we have all three cases of menuTrigger as examples?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO I feel like the description before the example is sufficient, happy for others to weigh in

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

In general looks really good. One question, then happy to approve


It is important to note that you don't have to control every single aspect of a ComboBox. If you decide to only control a single property of the
ComboBox, be sure to provide the change handler for that prop as well e.g. controlling `selectedKey` would require `onSelectionChange` to be passed to `useComboBox` as well.

Copy link
Member

Choose a reason for hiding this comment

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

Should we expand on the fact that the filtering in the example is different than the previous one precisely because it's controlled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll add a little blurb

@adobe-bot
Copy link

Build successful! 🎉

ktabors
ktabors previously approved these changes Jan 22, 2021
Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

LGTM


## Anatomy

A ComboBox consists of a label, an input which displays the current value, a listbox popup, and a button
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A ComboBox consists of a label, an input which displays the current value, a listbox popup, and a button
A ComboBox consists of a label, an input which displays the current value, a listbox popup, and an optional button


### Uncontrolled

This base example uses an `<input>` element for the ComboBox text input and a `<button>` element for the ComboBox menu trigger. A `<span>`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This base example uses an `<input>` element for the ComboBox text input and a `<button>` element for the ComboBox menu trigger. A `<span>`
This example uses an `<input>` element for the ComboBox text input, and a `<button>` element for the ComboBox menu trigger. A `<span>`

// to allow screen reader users to dismiss the popup easily.
return (
<div {...overlayProps} ref={popoverRef}>
<DismissButton onDismiss={() => state.close()} />
Copy link
Member

Choose a reason for hiding this comment

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

In RSP, we have only a single DismissButton. I guess because when you're at the start, you can navigate back to the input? Should we match?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'll remove the extra one

);
}

<ComboBox label="Favorite Animal" defaultSelectedKey="red panda">
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should not have a default selected key? It's weird to click the button and only see one option...


The following example shows how you would create a controlled ComboBox, controlling everything from the selected value (`selectedKey`)
to the combobox options (`items`). By passing in `inputValue`, `selectedKey`, `isOpen`, and `items` to the `useComboBoxState` hook you can control
what exactly your ComboBox should display. For example, note that the item filtering for the controlled ComboBox below now follows a "starts with"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
what exactly your ComboBox should display. For example, note that the item filtering for the controlled ComboBox below now follows a "starts with"
exactly what your ComboBox should display. For example, note that the item filtering for the controlled ComboBox below now follows a "starts with"

* Keyboard support for opening the ComboBox menu using the arrow keys, including automatically focusing
the first or last item accordingly
* Virtual focus management for ComboBox menu option navigation
* VoiceOver announcement enhancements for option focusing, filtering, and selection
Copy link
Member

Choose a reason for hiding this comment

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

I'd call these workarounds rather than enhancements... maybe something like this?

Suggested change
* VoiceOver announcement enhancements for option focusing, filtering, and selection
* Custom localized announcements for option focusing, filtering, and selection using an ARIA live region to work around VoiceOver bugs

It also holds state to track if the popup is open, if the ComboBox is focused, and the current input value.
For more information about selection, see [Selection](/react-stately/selection.html).

## Usage
Copy link
Member

Choose a reason for hiding this comment

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

I think we should split this into two sections: The "Example" section like we have on other aria pages which shows how to build the component using our hooks, and a "Usage" section that shows how to use the built component. Uncontrolled and Controlled can be under usage, but the code for the first example should be under "Example". The usage section should basically be a version of what we have on the spectrum docs, but without the spectrum-specific parts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I've moved the first example and accompanying text to a "Example" section and added a Uncontrolled section to "Usage"

setInputValue(value: string): void,
/** Function that when called handles updating the ComboBox's input value and selected key. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** Function that when called handles updating the ComboBox's input value and selected key. */
/** Selects the currently focused item and updates the input value. */

@@ -48,6 +57,12 @@ function isAppleDevice() {
: false;
}

/**
* Provides the behavior and accessibility implementation for a ComboBox component.
* A ComboBox combines a text entry with a picker menu, allowing users to filter longer lists to only the selections matching a query.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* A ComboBox combines a text entry with a picker menu, allowing users to filter longer lists to only the selections matching a query.
* A ComboBox combines a text input with a listbox, allowing users to filter a list of options to items matching a query.

## Internationalization

`useComboBox` handles some aspects of internationalization automatically.
For example, the item focus, count, and selection VoiceOver announcements are formatted to match the current locale.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For example, the item focus, count, and selection VoiceOver announcements are formatted to match the current locale.
For example, the item focus, count, and selection VoiceOver announcements are localized.


## Features

There is no native element to implement a ComboBox in HTML. `useComboBox` helps achieve accessible ComboBox components that can
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
There is no native element to implement a ComboBox in HTML. `useComboBox` helps achieve accessible ComboBox components that can
A ComboBox can be built using the [&lt;datalist&gt;](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/datalist) HTML element, but this is very limited in functionality and difficult to style. `useComboBox` helps achieve accessible ComboBox components that can

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

devongovett
devongovett previously approved these changes Jan 26, 2021
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Looks good. Will send a separate PR for the anatomy diagram.

DO NOT MERGE or it'll get deployed immediately. 😉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

dannify
dannify previously approved these changes Feb 10, 2021
@adobe-bot
Copy link

Build successful! 🎉

* Reuse the same example instead of duplicating

* Add anatomy and a couple more examples

* Updates

* Add select to the pickers category with combobox
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@devongovett devongovett merged commit 1d1d6a6 into main Feb 11, 2021
@devongovett devongovett deleted the docs_combobox_hooks branch February 11, 2021 15:56
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 documentation for Combobox hooks
6 participants