va-combo-box: add option groups functionality #1478

oleksii-morgun · 2025-02-04T14:03:24Z

Chromatic

https://combobox-option-groups--65a6e2ed2314f7b8f98609d8.chromatic.com

Configuring this pull request

Link to any related issues in the description so they can be cross-referenced.
Add the appropriate version patch label (major, minor, patch, or ignore-for-release).
- See How to choose a version number for guidance.
- Use ignore-for-release if files haven't been changed in a component library package. (ie. only Storybook files)
DST Only: Increment the /packages/core version number if this will be the last PR merged before a release.
Complete all sections below.
Delete this section once complete

Description

https://www.figma.com/design/uddjxccqvUyz5FSg1aD5wi/Z---Triage-Group-Flows?node-id=2095-166308&t=N9EvV05ND53hcKNY-0

adding support for optgroup tag that is expected to behave similar to select dropdown optgroup
optgroup is transformed to a <li> in a combobox list
optgroup receives custom distinct styling
optgroup is not selectable or searchable
child option elements of an optgroup receive aria-describedby of an optgroup they are associated with
child option element that matches the search query is displayed along with its parent optgroup element

QA Checklist

Component maintains 1:1 parity with design mocks
Text is consistent with what's been provided in the mocks
Component behaves as expected across breakpoints
Accessibility expert has signed off on code changes (if applicable. If not applicable provide reason why)
Designer has signed off on changes (if applicable. If not applicable provide reason why)
Tab order and focus state work as expected
Changes have been tested against screen readers (if applicable. If not applicable provide reason why)
New components are covered by e2e tests; updates to existing components are covered by existing test suite
Changes have been tested in vets-website using Verdaccio (if applicable. If not applicable provide reason why)

Screenshots

Acceptance criteria

QA checklist has been completed
Screenshots have been attached that cover desktop and mobile screens

Definition of done

Documentation has been updated, if applicable
A link has been provided to the originating GitHub issue (or connected to it via ZenHub)

oleksii-morgun · 2025-02-07T14:16:07Z

@BobbyBaileyRB has conducted a11y testing on our end as well

Andrew565 · 2025-02-07T16:00:42Z

packages/storybook/stories/va-combo-box-uswds.stories.jsx

@@ -46,6 +46,27 @@ const defaultArgs = {
  ],
 };

+const optGroupArgs = {


Please move this down to just before the export const OptGroups below so that it's more obvious that these args go with that template.

Andrew565 · 2025-02-07T16:01:22Z

packages/storybook/stories/va-combo-box-uswds.stories.jsx

+        <option value="Potatoes">Potatoes</option>
+      </optgroup>
+      <option value="sweet-potatoes">Sweet Potatoes</option>
+      <option value="cherry">Cherry</option>


I believe most people would be likely to associate "Cherry" and "Clementine" as "Fruits", not "Vegetables".

In this case, Sweet potatoes, Cherry, and Clementine are intended to fall in neither of those two categories to demonstrate that options can be either nested or not. Maybe I just need to pick examples that are distinctively different from those categories. I also wanted to pick some non nested values outside of an <optgroup> that have matching segments, to demonstrate filter functionality too

Andrew565 · 2025-02-07T16:02:35Z

packages/storybook/stories/va-combo-box-uswds.stories.jsx

@@ -115,3 +136,8 @@ WithMessageAriaDescribedBy.args = {
  ...defaultArgs,
  messageAriaDescribedby: 'This is example aria message',
 };
+
+export const OptGroups = Template.bind({});


Please expand the name of this template variable to "OptionGroups", as Storybook will automatically expand this to "Option Groups". "Opt Groups" is not "obvious" enough, in my opinion.

Andrew565 · 2025-02-07T16:06:06Z

packages/web-components/src/components/va-combo-box/va-combo-box-library.js

@@ -368,6 +370,8 @@ const noop = () => {};
    return new RegExp(find, 'i');
  };

+  const isDataOptGroup = option => option.getAttribute('data-optgroup') !== null;


This should probably be expanded to include && option.getAttribute('data-optgroup') !== false.

now that I am giving this another look, it probably should just be option.getAttribute('data-optgroup') === 'true';

Andrew565 · 2025-02-07T16:09:10Z

packages/web-components/src/components/va-combo-box/va-combo-box-library.js

+        if (!inputValue){
+          options.push(optionEl);
+          continue;
+        }
+
+        if (
+          inputValue &&
+          regex.test(optionEl.text) &&
+          optionEl.getAttribute('data-optgroup') !== 'true' 
+        ) {
+          if (
+            optionEl.getAttribute('data-optgroup-option') === 'true' &&
+            parentOptGroupId !== optionEl.getAttribute('aria-describedby')
+          ) {
+            parentOptGroupId = optionEl.getAttribute('aria-describedby');
+            const parentOptgroupEl = selectEl.querySelector(
+              '#' + parentOptGroupId,
+            );
+            options.push(parentOptgroupEl);
+          }
+          options.push(optionEl);
+        }


Can you add some comments to help make it more obvious what this code is doing?

micahchiang

Hey @oleksii-morgun! Thanks for working on this some more, we really appreciate the contribution. When you have a spare moment, would you mind leaving some review comments around the stuff going on va-combo-box-library.js and va-combo-box.tsx, just so we have a little more context? Thanks!

LWWright7

The current deployment of Storybook shows that when a user selects the dropdown arrow to look at the available options, the focus ring moves to the first item in the list and is being highlighted in the focus default yellow #FACE00

However, with this updated link it appears that the focus color has changed to black:

Correct me if I'm wrong, but I believe this should be in the focus yellow color mentioned above.

I'm guessing that the main reason for this is because, when an item from the list is selected and the user reengages the dropdown button, the highlighted option is shown in dark blue and if the black default color were used, I'm not sure it would pass a11y due to lack of contrast with the dark colors used, as shown here:

But perhaps @rsmithadhoc could provide some more insight on this particular issue?

danbrady · 2025-02-07T17:21:50Z

Great work, Alex! The design looks great. One minor suggestion, can we add a bit more left padding on the items within the optgroups? If you can't see the optgroup header, extra padding will help differentiate the items that are and are not part of an optgroup. It's currently 16px, but I'm leaning towards 24px.

Alternatively, maybe the last item in an optgroup gets a thicker border?

rsmithadhoc · 2025-02-10T17:43:52Z

The current deployment of Storybook shows that when a user selects the dropdown arrow to look at the available options, the focus ring moves to the first item in the list and is being highlighted in the focus default yellow #FACE00

However, with this updated link it appears that the focus color has changed to black:

Correct me if I'm wrong, but I believe this should be in the focus yellow color mentioned above.

I'm guessing that the main reason for this is because, when an item from the list is selected and the user reengages the dropdown button, the highlighted option is shown in dark blue and if the black default color were used, I'm not sure it would pass a11y due to lack of contrast with the dark colors used, as shown here:

But perhaps @rsmithadhoc could provide some more insight on this particular issue?

@LWWright7
I'm not completely sure on this one, so I'm open to other opinions, but here are my thoughts:

The version in the PR matches USWDS.
The black outline shows when you are not focused on the options, but focus is on the input (and it has the yellow outline). The black rectangle acts as a current selection indicator, rather than a focus indicator.
The same holds for the black and dark blue, nothing is focused at that point. In that case, I would say the boundary of that rectangle (black) should have a 3:1 contrast ratio to the adjacent background (white), which it does.
- The black outline against the dark blue rectangle is also just above a 3:1 ratio. I'm not sure if that would violate anything if it were lower, but I think it passes the minimum threshold if that were the case.
When navigating the options, it shows the yellow focus indicator correctly.
Having the double yellow might confuse when the option itself is not currently being focused.

For those reasons, I think the current version should be fine.

oleksii-morgun · 2025-02-11T21:16:16Z

Hey @oleksii-morgun! Thanks for working on this some more, we really appreciate the contribution. When you have a spare moment, would you mind leaving some review comments around the stuff going on va-combo-box-library.js and va-combo-box.tsx, just so we have a little more context? Thanks!

definitely. va-combo-box-library.js is mostly inherited from USWDS with an exception of few tweaks and what is in this PR to support optgroup - adding those comments in here .

oleksii-morgun · 2025-02-11T21:20:29Z

re: #1478 (review)

@LWWright7 here is a reference to a thread where this was brought up by @BobbyBaileyRB and it seems to align with @rsmithadhoc point. But I am open to your collective guidance here

oleksii-morgun · 2025-02-11T21:23:54Z

re: #1478 (comment)

@JoelCalumpong any concerns from Design perspective? I know we are considering this primarily for recipient list where we are trying to save as much real estate as possible.

oleksii-morgun · 2025-02-12T15:29:19Z

I pushed updates addressing some concerns and including feedback followup from all your input. Only item pending is the padding confirmation by our design team @JoelCalumpong. Other than that, should be ready for re-review when y'all have. a chance please

oleksii-morgun added 11 commits January 31, 2025 15:45

render optgroups

de2b888

handle groups in list

a8cd569

styling

c99b1ca

handle nav keys

d305430

handle selecting first option

778a825

handle aria for option in optgroups

2344925

handle filtering

71c0438

add storybook for testing

9bc843e

update storybook label

78fefb7

active option border color

1c0b2f4

unit tests

c954823

oleksii-morgun added the minor For a minor Semantic Version change label Feb 6, 2025

oleksii-morgun marked this pull request as ready for review February 6, 2025 21:34

oleksii-morgun requested a review from a team as a code owner February 6, 2025 21:34

jamigibbs requested a review from a team February 7, 2025 16:01

jamigibbs added patch Patch change in semantic versioning and removed minor For a minor Semantic Version change labels Feb 7, 2025

jamigibbs changed the title ~~Combobox option groups~~ va-combo-box: add option groups functionality Feb 7, 2025

Andrew565 requested changes Feb 7, 2025

View reviewed changes

micahchiang reviewed Feb 7, 2025

View reviewed changes

LWWright7 requested changes Feb 7, 2025

View reviewed changes

oleksii-morgun added 2 commits February 11, 2025 16:32

feedback follow up

72366f8

e2e update

094a142

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

va-combo-box: add option groups functionality #1478

va-combo-box: add option groups functionality #1478

oleksii-morgun commented Feb 4, 2025 •

edited

Loading

oleksii-morgun commented Feb 7, 2025

Andrew565 Feb 7, 2025

oleksii-morgun Feb 11, 2025

Andrew565 Feb 7, 2025

oleksii-morgun Feb 11, 2025

Andrew565 Feb 7, 2025

Andrew565 Feb 7, 2025

oleksii-morgun Feb 11, 2025

Andrew565 Feb 7, 2025

micahchiang left a comment

LWWright7 left a comment

danbrady commented Feb 7, 2025

rsmithadhoc commented Feb 10, 2025

oleksii-morgun commented Feb 11, 2025

oleksii-morgun commented Feb 11, 2025

oleksii-morgun commented Feb 11, 2025

oleksii-morgun commented Feb 12, 2025

va-combo-box: add option groups functionality #1478

Are you sure you want to change the base?

va-combo-box: add option groups functionality #1478

Conversation

oleksii-morgun commented Feb 4, 2025 • edited Loading

Chromatic

Configuring this pull request

Description

QA Checklist

Screenshots

Acceptance criteria

Definition of done

oleksii-morgun commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micahchiang left a comment

Choose a reason for hiding this comment

LWWright7 left a comment

Choose a reason for hiding this comment

danbrady commented Feb 7, 2025

rsmithadhoc commented Feb 10, 2025

oleksii-morgun commented Feb 11, 2025

oleksii-morgun commented Feb 11, 2025

oleksii-morgun commented Feb 11, 2025

oleksii-morgun commented Feb 12, 2025

oleksii-morgun commented Feb 4, 2025 •

edited

Loading