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

feat: combobox filtering and autocomplete #1125

Merged
merged 36 commits into from
Aug 3, 2020
Merged

Conversation

prsdthkr
Copy link
Member

@prsdthkr prsdthkr commented Jul 16, 2020

Description

In this change, we make the Combobox filterable and enable autocomplete capabilities.
We have tried to align with the specification here: ARIA 1.1 Combobox with Listbox Popup Examples
There is are some minor deviations from the specification because of the way we filter the Combobox options.

QA:
https://deploy-preview-1125--fundamental-react.netlify.app/?path=/story/component-api-comboboxinput--dev

fixes #1019

@prsdthkr prsdthkr self-assigned this Jul 16, 2020
@prsdthkr prsdthkr added the WIP Work in Progress label Jul 16, 2020
@netlify
Copy link

netlify bot commented Jul 16, 2020

Deploy preview for fundamental-react ready!

Built with commit a62e15b

https://deploy-preview-1125--fundamental-react.netlify.app

@skvale
Copy link
Contributor

skvale commented Jul 22, 2020

is this still a WIP or ready to be reviewed?

@prsdthkr
Copy link
Member Author

prsdthkr commented Jul 22, 2020

is this still a WIP or ready to be reviewed?

Hi @skvale, feel free to review and QA. I'm in the process of writing unit tests for the interactions. Will mark it as ready for review post that.

@prsdthkr prsdthkr marked this pull request as ready for review July 23, 2020 16:53
@prsdthkr prsdthkr removed the WIP Work in Progress label Jul 23, 2020
@prsdthkr prsdthkr requested a review from a team July 23, 2020 16:56
@@ -39,7 +39,9 @@ export default {
};

export const ${componentName} = () => {
let storyNames = Object.keys(stories).filter(story => story !== 'default');
let storyNames = Object.keys(stories).filter(story => {
return story !== 'default' && story !== 'dev';
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea! Just in case other reviewers are wondering why there's so many visual test changes - this is why. I think it's a good idea to remove the dev stories from the visuals.

@jbadan
Copy link
Contributor

jbadan commented Jul 24, 2020

I see this a11y violation, but I don't really understand it. I looked up the values for aria-controls, https://www.w3.org/WAI/PF/aria-1.1/states_and_properties#aria-controls and it says "ID reference list: A list of one or more ID references." We use aria-controls like this everywhere so 🤷‍♀️ Maybe this is a case of the tool not understanding grey area.

Screen Shot 2020-07-24 at 7 07 34 AM

@jbadan
Copy link
Contributor

jbadan commented Jul 24, 2020

This is an issue for fundamental-styles, but I think the list should either be full width or the container should be limited. We should open an issue over there.

Screen Shot 2020-07-24 at 7 11 45 AM

@jbadan
Copy link
Contributor

jbadan commented Jul 24, 2020

This looks really good!!

  1. Should the Combobox open if I haven't typed anything yet and press the down key - like if I wanted to see all the options or is that not a thing?

  2. We should pass the compact size to List:

Screen Shot 2020-07-24 at 7 16 00 AM

  1. We need to pass autocomplete='nope' instead of autocomplete='off' to FormInput, let the constant battle with keeping it disabled begin 😆

Screen Shot 2020-07-24 at 7 29 36 AM

@prsdthkr
Copy link
Member Author

prsdthkr commented Jul 24, 2020

Thanks for the review, Jenna 😃

  • aria-controls a11y issue: this happens mostly because the Listbox that the id is referring to is non-existent until opened. So if you re-run the a11y tests on opening the listbox - the error should disappear. To re-run the tests click on the ✓ Tests completed thingy below. I don't know if there is a way to overcome this violation.

  • autocomplete=nope introduces a new a11y violation 🙄 Ensure the autocomplete attribute is correct and suitable for the form field. Is this acceptable?

  • Should the Combobox open if I haven't typed anything yet and press the down key - like if I wanted to see all the options or is that not a thing?

    For this, I'll try to align with the ARIA example. There, the Combobox opens like that only in the 3rd example.

  • List width styling issue seems to be with the fd-list--dropdown class. Removing it has fixed it for now. I'll create I created an issue in fd-styles for this: Full width Combobox list fundamental-styles#1295

  • We should pass the compact size to List

    good catch 👍

@jbadan
Copy link
Contributor

jbadan commented Jul 27, 2020

@prsdthkr I think you're right about the autocomplete issue. Maybe we put a smaller version of this https://hig.concur.com/pages/develop/web/components/support.html#autocomplete-attribute-issue, to explain why that might happen.
Then we can do something like this:

<FormInput
    autocomplete='off'
    {...inputProps}
   {...labelProps}
   ....
/>

The consumer then has full control to override our autocomplete attribute to whatever works at the time.

Copy link
Contributor

@jbadan jbadan left a comment

Choose a reason for hiding this comment

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

LGTM 🚢 Great work!

@prsdthkr prsdthkr merged commit d84ea0b into master Aug 3, 2020
@prsdthkr prsdthkr deleted the feat/type-ahead-combobox branch August 3, 2020 22:37
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.

ComboboxInput: should filter based on FormInput input
3 participants