-
Notifications
You must be signed in to change notification settings - Fork 108
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(chips-combobox): add component #2062
Conversation
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.
LGTM overall I think. Couple of minor comments but I think its good to go
interface ChipsComboboxInput extends Omit<Marko.Input<"input">, `on${string}`> { | ||
expanded?: boolean; | ||
fluid?: boolean; | ||
error?: boolean; | ||
"list-selection"?: "manual" | "automatic"; | ||
options: string[]; | ||
selected?: string[]; | ||
roledescription?: AttrString; | ||
"a11y-delete-button"?: AttrString; | ||
"on-expand"?: () => void; | ||
"on-collapse"?: () => void; | ||
"on-change"?: (event: ChipsComboboxEvent) => void; | ||
} |
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.
Lets put this in index.marko like we had discussed with Dylan.
</ul> | ||
</if> | ||
<ebay-combobox | ||
class=["chips-combobox__combobox", inputClass] |
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.
Generally we want to put the extra classes on the root. The reason is sometimes users might want to be able to use those classes as selectors.
@@ -9,8 +9,8 @@ | |||
"compilerOptions": { | |||
"allowSyntheticDefaultImports": true, | |||
"incremental": true, | |||
"lib": ["dom"], | |||
"module": "commonjs", | |||
"lib": ["dom", "ESNext"], |
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.
Lets push this change directly to master or to 13.1.0. Lets not have it part of this PR
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.
Nice work! The code itself looks good.
There appear to be a11y
violations. Some, for example, the first two about forms
may not be legitimate, but others likely are.
@ianmcburnie, @saiponnada , thoughts?
Maybe we missed a couple of these in Skin?

For isolated view this should be fine |
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.
It looks like combobox
has the same issues marked as "violations." If we determine, these are issues, we can fix them together at some later point.
The rest looks good.
Description
chips-combobox
componentchips
andcombobox
, but none are breakingScreenshots