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

fix(combo-box): support v10 input controls #2241

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Mar 29, 2019

Closes #2240

This PR adds styles for the v10 combo box selection button and refactors multiselect selection button styles

Related:

https://github.com/IBM/carbon-components-react/issues/2109

https://github.com/IBM/carbon-components-react/issues/2113

Changelog

New

  • add styles for v10 combo box input controls

Changed

  • nest warning icons within list box field

Testing / Reviewing

  • Ensure listbox is not visually broken
  • Ensure multiselect is not visually broken
  • Ensure combobox is not visually broken

@netlify
Copy link

netlify bot commented Mar 29, 2019

Deploy preview for the-carbon-components ready!

Built with commit a5ffad8

https://deploy-preview-2241--the-carbon-components.netlify.com

@emyarod emyarod requested a review from a team March 29, 2019 17:30
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Looks like the invalid states are a little off.

  • Top one either missing the clear x or should be closet to the chevron
  • Bottom one is missing the icons

image

@emyarod emyarod force-pushed the 2240-support-v10-combobox-input-controls branch from c66db66 to b7e213b Compare April 1, 2019 14:43
@emyarod
Copy link
Member Author

emyarod commented Apr 1, 2019

both points should be addressed now @aagonzales

@emyarod emyarod force-pushed the 2240-support-v10-combobox-input-controls branch 2 times, most recently from aa9fcc2 to 121ca54 Compare April 2, 2019 13:05
@emyarod
Copy link
Member Author

emyarod commented Apr 2, 2019

feedback from @carmacleod on the React component: https://github.com/IBM/carbon-components-react/issues/1763#issuecomment-476252889

Some notes:

  • delete tabindex="0" from both of the outer divs, i.e. only the input should take focus. So all keyboard events need to be on the input.
  • the role="button" needs to go on the div containing the triangle icon. Do not add a tabindex. Mouse events can go on this, but not keyboard events.
  • the div.bx--list-box__field should have role="combobox" and aria-haspopup="listbox", aria-expanded="true/false", and aria-owns="unique-id-of-div.bx--list-box__menu" (note: if using component-user-provided label for id, make sure id does not have spaces).
  • the div.bx--form-item.bx--combo-box.bx--list-box should not have any role or any ARIA related markup - it's just a generic container.
  • the input could have role="textbox" (which is the default for an input type="text", so technically, you can leave the role off), and it needs aria-controls="unique-id-of-div.bx--list-box__menu" (same id as for the aria-owns on the combobox div), aria-autocomplete="list" (because typing in the input filters the list), and aria-label="Choose an item" (delete aria-label="ListBox input field"). The input also needs aria-activedescendant, but your code there seems good. :)
  • the div.bx--list-box__menu needs role="listbox" and the same aria-label as the input (i.e. aria-label="Choose an item").

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.

Changes:

  • There needs to be a 1px border bottom rule with ui-04 color when the dropdown is closed.

Screen Shot 2019-04-02 at 11 44 06 AM

  • There needs to be a 1px border bottom rule with ui-03 color when the dropdown is open.

Screen Shot 2019-04-02 at 11 44 43 AM

  • There needs to be an error field border around the examples that are showing an error state. 2px support-01 color.

  • The close icon needs to be icon-02 color.

  • If you full render, all of the open dropdowns merge into eachother because you can't close them, i don't know if that was intended.

@emyarod emyarod requested a review from laurenmrice April 2, 2019 18:13
@carbon-design-system carbon-design-system deleted a comment from netlify bot Apr 2, 2019
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.

  • The chevron icon needs to be icon-01. Only the close icon needs to be icon-02.

Screen Shot 2019-04-02 at 2 03 34 PM

  • When the dropdown is open the bottom border rule of the label section needs to be ui-03. It is currently ui-04. When the dropdown is closed then the rule should be ui-04.

Screen Shot 2019-04-02 at 2 03 26 PM

@emyarod
Copy link
Member Author

emyarod commented Apr 2, 2019

it looks like the text input styles were overriding the listbox styles there, should be fixed @laurenmrice

@emyarod emyarod requested a review from laurenmrice April 2, 2019 20:06
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 👍

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 basically, just one thing - Thanks alot @emyarod!

@@ -129,10 +247,50 @@
</div>
</div>
</div>
<div style="margin-bottom:10rem"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding a markup for spacer, could we do a devenv-specific style?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I've made that change now

@emyarod emyarod force-pushed the 2240-support-v10-combobox-input-controls branch from 0e48f52 to a5ffad8 Compare April 2, 2019 21:37
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.

Thanks @emyarod for the update!

@asudoh asudoh merged commit b5584bb into carbon-design-system:master Apr 2, 2019
@emyarod emyarod deleted the 2240-support-v10-combobox-input-controls branch April 3, 2019 14:12
@vpicone vpicone mentioned this pull request Apr 8, 2019
@joshblack joshblack mentioned this pull request Apr 9, 2019
@carbon-bot carbon-bot added package: react carbon-components-react priority: high severity: 1 https://ibm.biz/carbon-severity type: a11y ♿ labels May 9, 2019
@carbon-bot
Copy link
Contributor

Hi there! 👋 If you're wondering why this issue was moved, we're currently updating our repo structure so that every package is found in the same project.

This should not have any impact for you, but we wanted to give you a heads up in case you were wondering what is going on. If you have any questions, feel free to reach out to us on Slack or contact us at: [email protected]. Thanks!

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.

Support v10 combobox input controls
6 participants