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): add ability to group options #639

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kevinbuhmann
Copy link
Member

@kevinbuhmann kevinbuhmann commented Apr 14, 2023

Disclaimer

  • This feature has not been reviewed for accessibility.
  • This is just me proposing a possible feature. It might never be released.

PR Checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

Feature

What is the current behavior?

The combobox does not a built-in way to group options. Attempts to do this on the "consumer" side are hacky and and not fully accessible.

Issue Number: #635

What is the new behavior?

The combobox allows grouping options.

I used this example as guide: https://www.w3.org/WAI/ARIA/apg/patterns/listbox/examples/listbox-grouped/

Does this PR introduce a breaking change?

No.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 14, 2023

👋 @kevinbuhmann,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, checkout out a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal #clarity-support Slack channel

Thank you,

🤖 Clarity Release Bot

@kevinbuhmann kevinbuhmann force-pushed the kevin/combobox-grouped-options branch from 4c68b43 to 66f4698 Compare April 14, 2023 17:32
@github-actions

This comment was marked as outdated.

@kevinbuhmann kevinbuhmann force-pushed the kevin/combobox-grouped-options branch 5 times, most recently from bc2f7f1 to d37b026 Compare April 14, 2023 18:21
Copy link
Contributor

@Jinnie Jinnie left a comment

Choose a reason for hiding this comment

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

I like it!

A11y, UX, PM and I sign it "approved".

'[style.display]': 'clrOptionItems.hasResults ? undefined : "none"',
},
template: `
<span class="clr-option-group-label" role="presentation">{{ label }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really sure about this, but it will probably be covered by Amy's suggestion for ariaLabeledBy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah.

@@ -98,7 +98,7 @@ export class ClrOptions<T> implements AfterViewInit, LoadingListener, OnDestroy
@Input('id') optionsId: string;

_items: QueryList<ClrOption<T>>;
@ContentChildren(ClrOption)
@ContentChildren(ClrOption, { descendants: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

I really hope no-one tries to nest comboboxes :D

Copy link
Contributor

Choose a reason for hiding this comment

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

It can actually solve some problems, when people try to wrap options in their own component.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will look into that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, nesting comboxes makes zero sense. So...

@kevinbuhmann kevinbuhmann force-pushed the kevin/combobox-grouped-options branch 6 times, most recently from ead224c to ace8f2c Compare April 18, 2023 14:55
@kumar-tadepalli
Copy link

Any update on this pull request merge?

@kumar-tadepalli
Copy link

Could this be prioritized for next minor version release?

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.

5 participants