Add uswds combo box #1373

oleksii-morgun · 2024-10-24T14:11:06Z

Chromatic

https://add-uswds-combo-box--65a6e2ed2314f7b8f98609d8.chromatic.com

Description

Related to department-of-veterans-affairs/vets-design-system-documentation#3154

Designs https://www.figma.com/design/uddjxccqvUyz5FSg1aD5wi/ZFeat---Triage-Group-Selection-Flows?node-id=1804-73207&node-type=canvas&t=Rhhpivvbmxpw3vw2-0

USWDS combobox: https://designsystem.digital.gov/components/combo-box/

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)

jamigibbs

Thank you so much @oleksii-morgun for working on this! This is going to be a really nice addition to the component library. I've just left a few questions and comments for discussion. I've also tagged our designer group to have a look too when they have a chance. 🙇🏼‍♀️

jamigibbs · 2024-11-01T14:16:18Z

packages/web-components/src/components/va-combo-box/va-combo-box.tsx

+            onChange={e => this.handleChange(e)}
+            disabled={disabled}
+          >
+            {this.populateOptions()}


The spacing for the dropdown options don't match USWDS. Are we able to sync that up with their style module?

It also looks like our input has rounded corners but they are square.

Storybook

USWDS Example*

jamigibbs · 2024-11-01T14:19:48Z

packages/web-components/src/components/va-combo-box/va-combo-box.scss

+@import '../../mixins/uswds-input-width.scss';
+@import '../../mixins/uswds-error-border.scss';
+
+


We diverge a little bit with USWDS and use a yellow border on focus instead of blue so we'll want to do that with the combobox as well.

Here is an example of the yellow focus border on va-input:

We have a mixin that you can import here should make that update for you:

@import '../../mixins/focusable.css';

jamigibbs · 2024-11-01T14:22:06Z

packages/web-components/src/components/va-combo-box/test/va-combo-box.e2e.ts

+    );
+  });
+
+  it('renders disabled', async () => {


At a minimum we'd like to see a test for every prop and custom event for the component. That means I'd expect a minimum of 12 or so tests here but I only see 7. Do you mind beefing up these tests a little bit more?

jamigibbs · 2024-11-01T14:26:13Z

packages/web-components/src/components/va-combo-box/va-combo-box.tsx

+
+/**
+ * @componentName Combo Box
+ * @maturityCategory dont_use


I wonder if this should be Use With Caution: Candidate? 🤔

Here are the guidelines for the maturity scale.

The Use With Caution: Candidate requirements say:

Design System Team and/or Design System Council have evaluated the proposal.

The Design System Team will be creating documentation for the component or pattern.

The component or pattern may be in limited use (i.e. it may have already been shipped by a team). This could also be known as: “In work”, “Draft”, “Beta”, or “Give it a go! YMMV”.

jamigibbs · 2024-11-01T14:29:37Z

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

+Disabled.args = {
+  ...defaultArgs,
+  value: 'elderberry',
+  disabled: true,


I don't think we want to advertise that there is a disabled state. There are a11y concerns with this functionality.

Or maybe we don't want to have this functionality in the component at all? I will let the design and a11y reviewers weigh in on that though. cc @department-of-veterans-affairs/platform-design-system-designers @rsmithadhoc

Yeah, our stance on disabled states is that instead of disabling something, just don't show it if it serves no purpose. I'm okay with leaving it in for parity with USWDS, but just not advertising it.

jamigibbs · 2024-11-01T14:39:17Z

packages/web-components/src/components/va-combo-box/va-combo-box.tsx

+    composed: true,
+    bubbles: true,
+  })
+  componentLibraryAnalytics: EventEmitter;


I've been advised previously by @jestutt from platform analytics that we should not add analytics code to a component until we have coordinated with her. So I think we should remove any custom analytics code for now and we'll create a follow-up ticket to get it added.

Unless @oleksii-morgun your team needs it for your user testing?

jamigibbs · 2024-11-01T14:44:48Z

packages/web-components/src/components/va-combo-box/va-combo-box.tsx

+    }
+    const inputElement = this.el.shadowRoot.querySelector('input');
+    if (inputElement && this.error) {
+      inputElement.classList.add('usa-input--error');


Do you mind explaining a little more why displaying the error needs to be done in a component lifecycle instead of just using conditional markup in the render?

jamigibbs · 2024-11-01T14:45:16Z

packages/web-components/src/components/va-combo-box/va-combo-box.tsx

+    const labelElement = this.el.shadowRoot.querySelector('label');
+    if (comboBoxElement) {
+      comboBox.init(comboBoxElement, labelElement);
+      comboBox.on(comboBoxElement);


I'm not sure I understand why the label needs to be added in this way instead of just in the markup. Do you mind clarifying?

jamigibbs · 2024-11-01T14:45:42Z

packages/web-components/src/components/va-combo-box/va-combo-box.tsx

+      forceUpdate(this.el);
+    });
+    if (!this.label) {
+      throw new Error('va-combo-box: label is a required property');


I'm not sure about this approach for throwing an Error on required props. 🤔 Generally we will just allow the markup for the prop to display or not display.

jamigibbs · 2024-11-01T14:46:42Z

packages/web-components/src/components/va-combo-box/va-combo-box.tsx

+        action: 'change',
+        details: {
+          label: this.label,
+          selectLabel: this.value,


Analytics will need to be coordinated with @jestutt before we can add this.

oleksii-morgun · 2024-11-01T18:34:47Z

@jamigibbs thank you for a thorough review. I will have to get back to this the following week as I am out next week. And will look deeper into your suggestions.

rsmithadhoc · 2024-11-07T14:43:38Z

I'll have to give this a test with our accessibility testing criteria, ~~which I'll try to get to today or tomorrow,~~ thanks! Edit: actively taking a look.

oleksii-morgun added 18 commits October 18, 2024 10:22

initial component plot

9366703

tweaks and styling

9fb721e

adding label

4234bc1

adding disabled prop

cbb3448

adding defaultValue prop

fe3d3b2

placeholder prop

dca10e7

format and cleanup

19cccb5

handle label and value change

4de0269

handle required prop

1d76b2a

update doc attributes + cleanup

8b53d6e

update default value prop

b576a5f

handling hint text and aria

d49393a

docs

7d2e31e

build

fdf834f

styling cleanup

5daf7e9

update docs

f0dbdfb

e2e tests

6fb10f8

cleanup

1af41e7

oleksii-morgun marked this pull request as ready for review October 31, 2024 20:01

oleksii-morgun requested a review from a team as a code owner October 31, 2024 20:01

jamigibbs added the minor For a minor Semantic Version change label Nov 1, 2024

jamigibbs requested a review from a team November 1, 2024 13:19

jamigibbs reviewed Nov 1, 2024

View reviewed changes

jamigibbs requested a review from rsmithadhoc November 1, 2024 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add uswds combo box #1373

Add uswds combo box #1373

oleksii-morgun commented Oct 24, 2024 •

edited by jamigibbs

Loading

jamigibbs left a comment

jamigibbs Nov 1, 2024 •

edited

Loading

jamigibbs Nov 1, 2024

jamigibbs Nov 1, 2024 •

edited

Loading

jamigibbs Nov 1, 2024

jamigibbs Nov 1, 2024 •

edited

Loading

rsmithadhoc Nov 14, 2024

jamigibbs Nov 1, 2024

jamigibbs Nov 1, 2024

jamigibbs Nov 1, 2024 •

edited

Loading

jamigibbs Nov 1, 2024

jamigibbs Nov 1, 2024

oleksii-morgun commented Nov 1, 2024

rsmithadhoc commented Nov 7, 2024 •

edited

Loading

		@import '../../mixins/uswds-input-width.scss';
		@import '../../mixins/uswds-error-border.scss';

Add uswds combo box #1373

Are you sure you want to change the base?

Add uswds combo box #1373

Conversation

oleksii-morgun commented Oct 24, 2024 • edited by jamigibbs Loading

Chromatic

Description

QA Checklist

Screenshots

Acceptance criteria

Definition of done

jamigibbs left a comment

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

rsmithadhoc Nov 14, 2024

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024

Choose a reason for hiding this comment

jamigibbs Nov 1, 2024

Choose a reason for hiding this comment

oleksii-morgun commented Nov 1, 2024

rsmithadhoc commented Nov 7, 2024 • edited Loading

oleksii-morgun commented Oct 24, 2024 •

edited by jamigibbs

Loading

jamigibbs Nov 1, 2024 •

edited

Loading

jamigibbs Nov 1, 2024 •

edited

Loading

jamigibbs Nov 1, 2024 •

edited

Loading

jamigibbs Nov 1, 2024 •

edited

Loading

rsmithadhoc commented Nov 7, 2024 •

edited

Loading