feat(input-chips): new module

[ArtBlue]

Fixes #2159

• This PR contains CSS changes
• This PR does not contain CSS changes

Description

Created new input chips module that includes chips and a combobox selector.

Notes

WIP

Screenshots

Basic

With Label

With Helper Text

Disabled State

Error State

Expanded

Checklist

• I verify the build is in a non-broken state
• I verify all changes are within scope of the linked issue

• I regenerated all CSS files under dist folder
• I tested the UI in all supported browsers
• I did a visual regression check of the components impacted by doing a Percy build and approved the build
• I tested the UI in dark mode and RTL mode
• I added/updated/removed Storybook coverage as appropriate

[agliga]

Looks good. I think we could probably flatten some of this structure. It seems like there are too many levels and Im wondering if we can make it much simpler that way.

[LuLaValva]

It's a little bit weird that you need to click on the text section to get the combobox dropdown; I'm not sure if this would cause accessibility concerns, but as a user I'd expect that clicking anywhere inside the box besides on a chip would bring up the combobox and allow me to start typing.

[ArtBlue]

It's a little bit weird that you need to click on the text section to get the combobox dropdown; I'm not sure if this would cause accessibility concerns, but as a user I'd expect that clicking anywhere inside the box besides on a chip would bring up the combobox and allow me to start typing.

We actually talked about that previously. I don't see any a11y issues with that. In fact, trying to mimic that for screen readers might be the thing that causes an issue because to trigger that with a keyboard and screen reader, you'd need the chip container to get focus and be interactive in and of itself - as far as I know that's more likely to be problematic.

Visually speaking, I'm not convinced there's enough affordance there to expect a click anywhere inside the chip container to focus on the input and toggle the combobox. The reason why we insisted the design to have the placeholder text was actually for that very reason - not enough visual affordance and expectation of "turning on" the combobox when you click into the box. I suppose some users might expect that, but that would most likely be based on whether they've interacted with similar components. IMO this being far from comparable to any native component, I'm not sure that's a universal expectation. In fact, in certain situations (when there are enough chips to cover most of the box) doing that would not make a difference. We could definitely see if we can do some user testing and see if that's a gap.

That said, that could be a nice layered feature. Technically, it's another additive option for discoverability, which might be fine, but I doubt the lack of having it is an actual issue. My only major concern would be since the chip close buttons are not that big, someone missing the close target would prompt to open the combobox - not a major unrecoverable issue, but still...

Also, I'm not sure to what extent we should be building the interaction into Skin itself. IMO, as long as the interaction allows for all the UIs to surface for the docs to make visual sense (it does currently), Skin doesn't necessarily need to have the exhaustive set of interaction use cases. Those things should probably live in Ebay UI core.

[LuLaValva]

I'm not convinced there's enough affordance there to expect a click anywhere inside the chip container to focus on the input and toggle the combobox.

I agree this is a good progressive enhancement to add to eBayUI which doesn't necessarily need to be in skin, except for one exception. Right now clicking on the chevron on the top right has no effect, which was confusing for me so it will probably also be confusing for users

[ArtBlue]

I agree this is a good progressive enhancement to add to eBayUI which doesn't necessarily need to be in skin, except for one exception. Right now clicking on the chevron on the top right has no effect, which was confusing for me so it will probably also be confusing for users

Good catch. I was meaning to hook this up, but forgot. I'll add that.

[ArtBlue]

Ok, I hooked up the combobox expansion toggle on button click. One very important thing that I haven't finalized yet...

❗ IMPORTANT

Right now, the combobox list interaction is isolated to the input. I've been strongly considering decoupling that (removing combobox component classes?) and instead building that into .input-chips.

As part of my last change, I added .input-chips--expanded to allow cascading visual updates based on state to support that. However, right now the combobox list is still isolated and not integrated into that same cascading flow. I'm not sure what type of a11y implications a change like this might have, but it seem like overall the correct path forward.

[LuLaValva]

Is it proper BEM to use a single underscore?

Otherwise, looks awesome!

[LuLaValva]

Should we really be deviating this much from core listbox styles? I would imagine that the only part we should override is the positioning.

[ArtBlue]

If DS has the current specs where they have it, and we're trying to make it look how they want, which is a purely visual "preference," should the fact that we've chosen to use combobox under the hood be enough to overturn the visual specs that DS has landed on?

[ianmcburnie]

I still think there may be a better name than input-chips. I liked what we did for date-textbox, as the name lets us know it is a textbox specifically for dates. I wonder if we can do something similar here and incorporate the underlying control (a combobox) in the name...?

[ianmcburnie]

Is it proper BEM to use a single underscore?

No. Should be double underscore.

[ianmcburnie]

FYI looks like a couple of stale dist files crept in after I build. I wonder if prettier config is not picking these up when staged.

[LuLaValva]

Looks great! Just a few minor comments.

[ianmcburnie]

Is there a reason we have deviated away from the default textbox width?

[ArtBlue]

Is there a reason we have deviated away from the default textbox width?

I don't think the Figma had any min/max/default widths. I can't verify it at the moment (getting a blank page ATM).

From the dev perspective, since this is a composite component, it made sense to have it expand into the container limitations and allow implementation-level layouts to control its dimensions.

I suppose, we can do what we do with textbox and the fluid modifier. I'll look into that.

[agliga]

Looks great overall. I think its really come a long way.
One comment, is there a way to make this interactive on the docs? We have other components which are interactive like combobox and select. It really adds to the whole feel if a user can actually click an item in chips combobox and see it change. Or hit x and see it removed. It doesn't have to be complex, but at least to see the interactions.

[ianmcburnie]

Nice job @ArtBlue And thanks for working with design to remove those early a11y barriers.

[ianmcburnie]

@ArtBlue Oops, just spotted what looks like a couple of unsynced dist files in your branch:

[agliga]

There are some errors when I run this locally

> cat lint.log

/Users/agliga/build/ebay/skin/docs/src/js/main.js
  549:21  error  'sItemSelected' is never reassigned. Use 'const' instead   prefer-const
  559:21  error  'sChipText' is never reassigned. Use 'const' instead       prefer-const
  560:21  error  'elChipParentLI' is never reassigned. Use 'const' instead  prefer-const

✖ 3 problems (3 errors, 0 warnings)
  3 errors and 0 warnings potentially fixable with the `--fix` option.

Also I see a lot of files being deleted when I run locally. Not sure if its because of merge or not.

[ArtBlue]

There are some errors when I run this locally

> cat lint.log

/Users/agliga/build/ebay/skin/docs/src/js/main.js
  549:21  error  'sItemSelected' is never reassigned. Use 'const' instead   prefer-const
  559:21  error  'sChipText' is never reassigned. Use 'const' instead       prefer-const
  560:21  error  'elChipParentLI' is never reassigned. Use 'const' instead  prefer-const

✖ 3 problems (3 errors, 0 warnings)
  3 errors and 0 warnings potentially fixable with the `--fix` option.

Wow! That is extremely opinionated for docs js. LOL. Ok, I'll change them.

Also I see a lot of files being deleted when I run locally. Not sure if its because of merge or not.

I'm not sure. The diff on the PR doesn't have that. Maybe it's something local?

[agliga]

Lets wait for the floating ui PR to be merged before merging this.