-
Notifications
You must be signed in to change notification settings - Fork 88
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
[WNMGDS-2871] Adds ds-autocomplete
component
#3257
Conversation
* Adds notes and scaffolding for ds-tabs and ds-tab-panel web components. * Adds functionalality to parse netested children into expected react node type. * Removes unnecessary recursive loop in parseChildren. * Further simplifies parseChildren implementation * Adds explicit definition of event structure for ds-change. Adds additional documentation to storybook. * Adds unit tests for ds-tabs component. * Removes an unnecessary console.log line. * Adds browser test snapshots. * Adds an entrypoint for the ds-tabs module. * Removed conflicting snapshot files * Revert "Adds browser test snapshots." This reverts commit b554c27. * Revert "Removed conflicting snapshot files" This reverts commit ae47751. * Adds ds-tabs and ds-tab-panel to web components entrypoint file. * Removes ds-tab-panel from web-components/index.ts * Adds snapshots for ds-tabs and storybook docs. * Removes debugging lines from stories.test.ts and expectNoAxeViolations.ts.
#3246) * Changed focus color from orchid to copper * reverted other token changes * Updated snapshots * Updated snapshots again * Ran yarn build:docs and reran snapshots --------- Co-authored-by: Mallory Iden <[email protected]>
'no-results-message': 'No results', | ||
name: 'ds-autocomplete', | ||
}, | ||
argTypes: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might want to add this property table: { disable: true, },
to the following items: textFieldLabel, textFieldHint and children so they don't appear in the attributes table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
autoFocus={parseBooleanAttr(autofocus)} | ||
clearSearchButton={parseBooleanAttr(clearSearchButton)} | ||
id={rootId} | ||
items={JSON.parse(items)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since items
is an optional prop, I'm wondering if there's a chance this could throw an error if items
is undefined
. Perhaps we could add a check or a fallback value to handle that scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah it crashes hard if no items are provided. But not providing any items kinda defeats the purpose of a combobox. Is the idea that these items can be loaded in async in some cases so we don't want to provide a fallback @kim-cmsds ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good lines of thought. I'll take a look at this use case and push up changes.
expectMenuToBeClosed(); | ||
}); | ||
|
||
// it("calls child TextField's event handlers", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticing that several unit tests are commented out. Is this intentional, or should we consider re-enabling or removing them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit tests that are commented out correspond to how the react component's API expects a <TextField />
component to be passed in as a child. The new, simpler web component API does not currently allow for that functionality. Instead, we're "flattening" the component and passing in a far less customizable text input here:
Lines 66 to 77 in 7735a39
<Autocomplete | |
{...otherProps} | |
label={menuHeading} | |
labelId={menuHeadingId} | |
autoFocus={parseBooleanAttr(autofocus)} | |
clearSearchButton={parseBooleanAttr(clearSearchButton)} | |
id={rootId} | |
items={JSON.parse(items)} | |
loading={parseBooleanAttr(loading)} | |
> | |
<TextField label={label} hint={hint} name="autocomplete" value={value} /> | |
</Autocomplete> |
I may spin up a follow-up ticket to see if there's a need to adjust the API to "unflatten" it and have it match the React component's API more closely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything's working really well! The only thing that is broken is the component bundle, and that's because it needs to use import
to signal to webpack that it contains side effects.
packages/design-system/src/components/web-components/ds-autocomplete/index.ts
Outdated
Show resolved
Hide resolved
…mplete/index.ts Co-authored-by: Patrick Wolfert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it on storybook as well as the astro example, and it looks good to me!
* initial autocomplete scaffolding * speculatively check for a wc text field * flatten autocomplete wc * a more complete autocomplete example * initial autocomplete unit test, snapshot * adds to test coverage * Clarifies text field hint for "no results" story * formats autocomplete and text field astro examples * comments out custom markup example * adds additional test coverage to ds-autocomplete example * updates storybook docs snapshots * updates astro web component script srcs * remove unnecessary comment * updates snapshots * updates examples snapshots * Update packages/design-system/src/components/web-components/ds-autocomplete/index.ts Co-authored-by: Patrick Wolfert <[email protected]> * updates storybook docs table per review * more safely check for undefined in various web components; adds test coverage * updates snapshots * Removes a proof-of-concept change * updates snaps --------- Co-authored-by: Patrick Wolfert <[email protected]>
Summary
WNMGDS-2871
Adds a new
ds-autocomplete
web component.How to test
yarn build
at the project root and then runyarn dev
from/examples/astro
Checklist
[WNMGDS-####] Title
or [NO-TICKET] if this is unticketed work.Type
(only one) label for this PR, if it is a breaking change, label should only beType: Breaking
Impacts
, multiple can be selected.If this is a change to code:
yarn test:unit:update
) and browser-test snapshots (yarn test:browser:update
)If this is a change to documentation: