-
Notifications
You must be signed in to change notification settings - Fork 77
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(autocomplete): add autocomplete component #10562
Conversation
…n-system into dris0000/autocomplete
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
# Conflicts: # packages/calcite-components/index.html # packages/calcite-components/src/components.d.ts # packages/calcite-components/stencil.config.ts
# Conflicts: # t9nmanifest.txt
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.
Code LGTM! Do you need additional manual testing or has that already been taken care of?
packages/calcite-components/src/components/autocomplete-item/autocomplete-item.tsx
Show resolved
Hide resolved
heading: "heading", | ||
iconEnd: "icon-end", | ||
iconStart: "icon-start", | ||
scale: (scale: Scale) => `scale--${scale}` as const, |
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.
Is this a new pattern we're using to avoid the more costly attribute selector?
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.
Its used in combobox-item
.
calcite-design-system/packages/calcite-components/src/components/combobox-item/resources.ts
Line 13 in e0f5a85
scale: (scale: Scale) => `scale--${scale}` as const, |
I copied it from there. Is that ok?
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.
Gotcha, yeah this was just a sidebar; I was curious if that's something we should be doing instead of using the attribute selector like this:
calcite-design-system/packages/calcite-components/src/components/icon/icon.scss
Lines 18 to 37 in e0f5a85
:host([scale="s"]) { | |
inline-size: $icon-size-s; | |
block-size: $icon-size-s; | |
min-inline-size: $icon-size-s; | |
min-block-size: $icon-size-s; | |
} | |
:host([scale="m"]) { | |
inline-size: $icon-size-m; | |
block-size: $icon-size-m; | |
min-inline-size: $icon-size-m; | |
min-block-size: $icon-size-m; | |
} | |
:host([scale="l"]) { | |
inline-size: $icon-size-l; | |
block-size: $icon-size-l; | |
min-inline-size: $icon-size-l; | |
min-block-size: $icon-size-l; | |
} |
I think either are fine, I just hadn't seen the scale--s
pattern in our codebase yet. After a quick grep I also found instances of scale-s
, e.g.
calcite-design-system/packages/calcite-components/src/components/tab/tab.scss
Lines 29 to 42 in e0f5a85
.scale-s { | |
--calcite-internal-tab-content-block-padding: var(--calcite-tab-content-block-padding, theme("spacing.1")); | |
@apply text-n2h; | |
} | |
.scale-m { | |
--calcite-internal-tab-content-block-padding: var(--calcite-tab-content-block-padding, theme("spacing.2")); | |
@apply text-n1h; | |
} | |
.scale-l { | |
--calcite-internal-tab-content-block-padding: var(--calcite-tab-content-block-padding, theme("spacing.[2.5]")); | |
@apply text-0h; | |
} |
Do we have any conventions on whether we should be using BEM in our CSS class names @jcfranco? It seems rather inconsistent between components.
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 agree .scale--s
would be better than .scale-s
to align with a standardized pattern. We don't really need BEM for specificity since our components are shadowed. Probably an issue to cleanup class names.
I do prefer classes over using attribute selectors because it forces those attributes to be reflected and can make some selectors quite complex.
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 agree
.scale--s
would be better than.scale-s
to align with a standardized pattern. We don't really need BEM for specificity since our components are shadowed. Probably an issue to cleanup class names.
I also prefer .scale--s
since it follows convention. I don't think it's worth an issue because the classes aren't exposed to users. But maybe we can recommend the block--modifier-value
naming convention for future PRs so the codebase doesn't diverge any further, cc @jcfranco.
I do prefer classes over using attribute selectors because it forces those attributes to be reflected and can make some selectors quite complex.
Yeah makes sense, thanks!
packages/calcite-components/src/components/autocomplete/autocomplete.e2e.ts
Show resolved
Hide resolved
packages/calcite-components/src/components/autocomplete/autocomplete.stories.ts
Show resolved
Hide resolved
packages/calcite-components/src/components/autocomplete/autocomplete.tsx
Outdated
Show resolved
Hide resolved
@benelan additional manual testing would be great if you have some time. |
I have to run off to an hour appointment but I can do some later today. I'll focus on form validation stuff. |
Related Issue: #10044
Summary
calcite-autocomplete
,calcite-autocomplete-item
, andcalcite-autocomplete-item-group
Tokens
Autocomplete
--calcite-autocomplete-background-color: Specifies the background color of the component.
--calcite-autocomplete-corner-radius: Specifies the component's corner radius.
--calcite-autocomplete-text-color: Specifies the text color of the component.
--calcite-autocomplete-input-prefix-size: Specifies the Input's prefix width, when present.
--calcite-autocomplete-input-suffix-size: Specifies the Input's suffix width, when present.
--calcite-autocomplete-input-background-color: Specifies the background color of the Input.
--calcite-autocomplete-input-border-color: Specifies the border color of the Input.
--calcite-autocomplete-input-corner-radius: Specifies the corner radius of the Input.
--calcite-autocomplete-input-shadow: Specifies the shadow of the Input.
--calcite-autocomplete-input-icon-color: Specifies the icon color of the Input.
--calcite-autocomplete-input-text-color: Specifies the text color of the Input.
--calcite-autocomplete-input-placeholder-text-color: Specifies the color of placeholder text in the Input.
--calcite-autocomplete-input-actions-background-color: Specifies the background color of Input's
clearable
element.--calcite-autocomplete-input-actions-background-color-hover: Specifies the background color of Input's
clearable
element when hovered.--calcite-autocomplete-input-actions-background-color-press: Specifies the background color of Input's
clearable
element when pressed.--calcite-autocomplete-input-actions-icon-color: Specifies the icon color of Input's
clearable
element.--calcite-autocomplete-input-actions-icon-color-hover: Specifies the icon color of Input's
clearable
element when hovered.--calcite-autocomplete-input-actions-icon-color-press: Specifies the icon color of Input's
clearable
element when pressed.--calcite-autocomplete-input-loading-background-color: Specifies the background color of the Input's
loading
element, when present.--calcite-autocomplete-input-loading-fill-color: Specifies the fill color of the Input's
loading
element, when present.--calcite-autocomplete-input-prefix-background-color: Specifies the background color of the Input's prefix, when present.
--calcite-autocomplete-input-prefix-text-color: Specifies the text color of the Input's prefix, when present.
--calcite-autocomplete-input-suffix-background-color: Specifies the background color of the Input's suffix, when present.
--calcite-autocomplete-input-suffix-text-color: Specifies the text color of the Input's suffix, when present.
Autocomplete item
--calcite-autocomplete-background-color: Specifies the background color of the component.
--calcite-autocomplete-description-text-color: Specifies the text color of the component's description.
--calcite-autocomplete-heading-text-color: Specifies the text color of the component's heading.
--calcite-autocomplete-text-color: Specifies the text color of the component.
Autocomplete item group
--calcite-autocomplete-background-color: Specifies the background color of the component.
--calcite-autocomplete-border-color: Specifies the border color of the component.
--calcite-autocomplete-text-color: Specifies the text color of the component.