Skip to content

Typeahead component with tdd approch - #185

Merged
vinodloha merged 33 commits into
developfrom
typeahead
Dec 23, 2019
Merged

vinodloha merged 33 commits into
developfrom
typeahead

Conversation

@RohitLuthra19

Copy link
Copy Markdown
Collaborator

No description provided.

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated
Comment thread lib/components/atoms/Button/Button.js
Comment thread lib/components/molecules/Typeahead/ListItem.js
Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated
Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated
Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated
Comment thread lib/components/molecules/Typeahead/Typeahead.story.js Outdated
Comment thread lib/components/molecules/Typeahead/tests/Typeahead.test.js

@vinodloha vinodloha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Small fixes required

@RohitLuthra19 RohitLuthra19 changed the title added typeahead component with tdd approch Typeahead component with tdd approch Dec 16, 2019

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated
@vinodloha

vinodloha commented Dec 19, 2019 •

Copy link
Copy Markdown
Collaborator

@RohitLuthra19 Please resolve conflicts.

Secondly, I think we are missing couple of use-cases.

  1. When showing more than 5 or 10 (Configurable) items we should put a vertical scroll on list items.
  2. How can we support no search results state?
  3. First item should not be selected by default.

Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated

@vinodloha vinodloha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tested the component, some more improvements required.

Comment thread lib/components/molecules/Typeahead/Typeahead.story.js Outdated

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

Comment thread lib/components/molecules/Typeahead/Typeahead.js Outdated

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

@vinodloha

Copy link
Copy Markdown
Collaborator

@RohitLuthra19 I tested your component with Voice Over and it could not read any of the search result items. Please test and fix.

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

]
}
type="search"
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

@accesslint accesslint Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

onChange={[Function]}
onKeyDown={[Function]}
type="search"
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like there's a label missing for this input. That makes it hard for people using screen readers or voice control to use the input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants