Skip to content
This repository has been archived by the owner on Oct 19, 2021. It is now read-only.

fix(ComboBox): match experimental spec #2158

Merged
merged 27 commits into from
Apr 17, 2019
Merged

fix(ComboBox): match experimental spec #2158

merged 27 commits into from
Apr 17, 2019

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Apr 4, 2019 •
edited
Loading

Closes IBM/carbon-components-react#2113

Closes #1763

Closes #2081

This PR adds the experimental combo box while also addressing a number of a11y and UX issues.

Changelog

New

  • menu option to show overflow text behavior in component story
  • require id for more accessible labels (affects all listbox components), related fix(DropdownV2): add ID prop #1109

Changed

  • fix extra tab stops (found in all listbox components)

Removed

  • unneeded or redundant attributes

Testing/Reviewing

Ensure combo box works as expected

Ensure there are no regressions with the listbox components

@emyarod emyarod added this to the v10-update milestone Apr 4, 2019
@emyarod
Copy link
Member Author

emyarod commented Apr 4, 2019

blocked by #2093 and carbon-design-system/carbon#2241. These changes have not yet been released, but testing can still be done locally via yarn link/npm link

@netlify
Copy link

netlify bot commented Apr 4, 2019 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit 18b6473

https://deploy-preview-2158--carbon-components-react.netlify.com

@emyarod emyarod mentioned this pull request Apr 4, 2019
Merged
2 tasks
@asudoh
Copy link
Contributor

asudoh commented Apr 4, 2019

Removed blocked label given both PRs that @emyarod marked as dependencies seem to have been merged.

@emyarod
Copy link
Member Author

emyarod commented Apr 4, 2019

@asudoh the only blocker is that we haven't cut a vanilla release which includes the changes from those PRs

@emyarod emyarod force-pushed the 2113-match-combobox-to-experimental-spec branch 2 times, most recently from f4c295b to 0488f63 Compare April 8, 2019 16:28
@emyarod emyarod marked this pull request as ready for review April 9, 2019 17:48
@emyarod emyarod force-pushed the 2113-match-combobox-to-experimental-spec branch from 0488f63 to 91dae77 Compare April 9, 2019 17:49
@emyarod emyarod requested a review from laurenmrice April 9, 2019 18:21
@emyarod emyarod force-pushed the 2113-match-combobox-to-experimental-spec branch 2 times, most recently from 5075255 to 13ce1c6 Compare April 9, 2019 18:48
laurenmrice
laurenmrice suggested changes Apr 9, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

When the dropdown is closed and there is an x icon, when you click the x icon, the drawer opens and the x goes away. If the dropdown is already closed, i think you should be able to click the x and the dropdown will remain closed.
Screen Shot 2019-04-09 at 4 20 06 PM

@emyarod
Copy link
Member Author

emyarod commented Apr 10, 2019

@laurenmrice it looks like right now we are intentionally opening the dropdown when the user clicks the x to clear selections (https://github.com/IBM/carbon-components-react/blob/master/src/components/ListBox/ListBoxSelection.js#L34-L36). Is this going to be a v10 only change or do we also want to modify the v9 behavior?

@emyarod emyarod force-pushed the 2113-match-combobox-to-experimental-spec branch from 13ce1c6 to 99f3b5c Compare April 10, 2019 21:35
@emyarod emyarod force-pushed the 2113-match-combobox-to-experimental-spec branch from 99f3b5c to e732b99 Compare April 11, 2019 18:56
@emyarod emyarod force-pushed the 2113-match-combobox-to-experimental-spec branch from e732b99 to 9d1177c Compare April 15, 2019 15:06
@laurenmrice
Copy link
Member

@emyarod Ok if this is happening in v9 lets just keep it as is for now and revisit this later.

@emyarod
Copy link
Member Author

emyarod commented Apr 17, 2019

@laurenmrice sounds good! this should be ready for review again then since no changes were made

laurenmrice
laurenmrice previously requested changes Apr 17, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

  • Only change is that we do not need a hover on the label because you can type into the field.
    Screen Shot 2019-04-17 at 12 01 46 PM

@laurenmrice laurenmrice dismissed their stale review April 17, 2019 17:05

actually dont think we need to omit hover

laurenmrice
laurenmrice approved these changes Apr 17, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice 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 👍

asudoh
asudoh approved these changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @emyarod!

@asudoh asudoh merged commit 2753040 into carbon-design-system:master Apr 17, 2019
@emyarod emyarod deleted the 2113-match-combobox-to-experimental-spec branch April 18, 2019 17:40
@vpicone vpicone mentioned this pull request Apr 30, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laurenmrice laurenmrice laurenmrice approved these changes

@asudoh asudoh asudoh approved these changes

Assignees
No one assigned
Labels
status: ready for review 👀 status: visual review 🔍
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emyarod @asudoh @laurenmrice