Skip to content
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(combobox, combobox-item): add description, shortHeading props and content-end slot #9771

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/calcite-components/src/components.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1365,6 +1365,10 @@ export namespace Components {
* Specifies the parent and grandparent items, which are set on `calcite-combobox`.
*/
"ancestors": ComboboxChildElement[];
/**
* A description for the component, which displays below the label.
*/
"description": string;
/**
* When `true`, interaction is prevented and the component is displayed with lower opacity.
*/
Expand Down Expand Up @@ -1408,6 +1412,10 @@ export namespace Components {
* The component's text.
*/
"textLabel": string;
/**
* The component's short label. When provided, the short label will be displayed in the component's selection. It is recommended to use 5 chars or less.
*/
"textLabelShort": string;
/**
* The component's value.
*/
Expand Down Expand Up @@ -9200,6 +9208,10 @@ declare namespace LocalJSX {
* Specifies the parent and grandparent items, which are set on `calcite-combobox`.
*/
"ancestors"?: ComboboxChildElement[];
/**
* A description for the component, which displays below the label.
*/
"description"?: string;
/**
* When `true`, interaction is prevented and the component is displayed with lower opacity.
*/
Expand Down Expand Up @@ -9247,6 +9259,10 @@ declare namespace LocalJSX {
* The component's text.
*/
"textLabel": string;
/**
* The component's short label. When provided, the short label will be displayed in the component's selection. It is recommended to use 5 chars or less.
*/
"textLabelShort"?: string;
/**
* The component's value.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
--calcite-combobox-item-spacing-unit-s: theme("spacing.1");
--calcite-combobox-item-spacing-indent: theme("spacing.2");
--calcite-combobox-item-selector-icon-size: theme("spacing.4");
--calcite-combobox-item-description-font-size: var(--calcite-font-size--3);
}

.scale--m {
Expand Down Expand Up @@ -48,20 +49,22 @@ ul:focus {

.label {
@apply text-color-3
focus-base
relative
box-border
flex
w-full
min-w-full
cursor-pointer
items-center
no-underline
duration-150
ease-in-out;
focus-base
relative
box-border
flex
w-full
min-w-full
cursor-pointer
items-center
no-underline
duration-150
ease-in-out;
@include word-break();
justify-content: space-around;
gap: var(--calcite-combobox-item-spacing-unit-l);
padding-block: var(--calcite-combobox-item-spacing-unit-s);
padding-inline: var(--calcite-combobox-item-spacing-unit-l);
padding-inline: var(--calcite-combobox-item-indent-value);
}

:host([disabled]) .label {
Expand All @@ -85,11 +88,6 @@ ul:focus {
shadow-none;
}

.title {
padding-block: 0;
padding-inline: var(--calcite-combobox-item-spacing-unit-l);
}

.icon {
@apply inline-flex
opacity-0
Expand All @@ -98,13 +96,8 @@ ul:focus {
color: theme("borderColor.color.1");
}

.icon--indent {
padding-inline-start: var(--calcite-combobox-item-indent-value);
}

.icon--custom {
margin-block-start: -1px;
padding-inline-start: var(--calcite-combobox-item-spacing-unit-l);
@apply text-color-3;
}

Expand Down Expand Up @@ -140,3 +133,25 @@ ul:focus {
color: var(--calcite-color-text-1);
background-color: var(--calcite-color-foreground-current);
}

.center-content {
display: flex;
flex-direction: column;
flex-grow: 1;
padding-block: 0;
}

.description {
font-size: var(--calcite-combobox-item-description-font-size);
}

:host([selected]),
:host(:hover) {
.description {
color: var(--calcite-color-text-2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will description and short-text need CSS vars for their color at some point?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

description might. WDTY @ashetland?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need a custom CSS prop, I'll add it in the component tokens PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make sense for the short text and I'm guessing we're doing that for description text across components?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so. I'll double check and add it to #8594.

}
}

.short-text {
color: var(--calcite-color-text-3);
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ import { getAncestors, getDepth, isSingleLike } from "../combobox/utils";
import { Scale, SelectionMode } from "../interfaces";
import { getIconScale } from "../../utils/component";
import { IconName } from "../icon/interfaces";
import { CSS } from "./resources";
import { CSS, SLOTS } from "./resources";

/**
* @slot - A slot for adding nested `calcite-combobox-item`s.
* @slot content-end - A slot for adding non-actionable elements after the component's content.
*/
@Component({
tag: "calcite-combobox-item",
Expand Down Expand Up @@ -59,6 +60,11 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
/** Specifies the parent and grandparent items, which are set on `calcite-combobox`. */
@Prop({ mutable: true }) ancestors: ComboboxChildElement[];

/**
* A description for the component, which displays below the label.
*/
@Prop() description: string;

/** The `id` attribute of the component. When omitted, a globally unique identifier is used. */
@Prop({ reflect: true }) guid = guid();

Expand All @@ -76,6 +82,15 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
/** The component's text. */
@Prop({ reflect: true }) textLabel!: string;

/**
* The component's short label.
*
* When provided, the short label will be displayed in the component's selection.
*
* It is recommended to use 5 chars or less.
*/
@Prop({ reflect: true }) textLabelShort: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can come up with a better name for this prop?

Something like snippet or summary? Or selected-label?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at some point we should rename textLabel to just label as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like snippet or summary? Or selected-label?

When specified this prop shows an abbreviated version of the label that is used in selection. The suggested names do not quite reflect this.

I think at some point we should rename textLabel to just label as well.

Agreed, but until then it might help associate both props by matching the existing name. There were other suggestions in the design spec, but textLabelShort was the shortest and most expressive (IMO). Open to alternatives. cc @SkyeSeitz @ashetland

Copy link
Contributor

@ashetland ashetland Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, I've got nothin' better than textLabelShort.

Long-term I do like the idea of renaming the main two props to label and description across the system. It's not super consistent atm. The primary text is often title or heading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just throwing this out there:

Instead of adding another prop we plan on renaming in the future, why not just deprecate textLabel for label and add description?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding another prop we plan on renaming in the future, why not just deprecate textLabel for label

We'd want to use heading since label is mostly used for accessible text, right?

Also wanted to point out that my PR title isn't quite accurate. It's a short heading/label we're adding, not a description. Will update.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driskull This example shows how the prop comes into play.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think something like shortHeading should be good. 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with moving towards a consistent heading & description paradigm across components accompanied by shortHeading when relevant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Thanks for the discussion, y'all! ✨💪✨

I'll submit an issue/PR to deprecate textLabel separately since it is out of scope.


/**
* Pattern for highlighting filter text matches.
*
Expand Down Expand Up @@ -189,7 +204,6 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
class={{
[CSS.custom]: !!this.icon,
[CSS.iconActive]: this.icon && this.selected,
[CSS.iconIndent]: true,
}}
flipRtl={this.iconFlipRtl}
icon={this.icon || iconPath}
Expand All @@ -207,15 +221,13 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
class={{
[CSS.icon]: true,
[CSS.dot]: true,
[CSS.iconIndent]: true,
}}
/>
) : (
<calcite-icon
class={{
[CSS.icon]: true,
[CSS.iconActive]: this.selected,
[CSS.iconIndent]: true,
}}
flipRtl={this.iconFlipRtl}
icon={iconPath}
Expand Down Expand Up @@ -250,19 +262,31 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
[CSS.active]: this.active,
[CSS.single]: isSingleSelect,
};
const depth = getDepth(this.el);
const depth = getDepth(this.el) + 1;

return (
<Host aria-hidden="true">
<InteractiveContainer disabled={disabled}>
<div
class={`container scale--${this.scale}`}
class={{
[CSS.container]: true,
[CSS.scale(this.scale)]: true,
}}
style={{ "--calcite-combobox-item-spacing-indent-multiplier": `${depth}` }}
>
<li class={classes} id={this.guid} onClick={this.itemClickHandler}>
{this.renderSelectIndicator(showDot, iconPath)}
{this.renderIcon(iconPath)}
<span class="title">{this.renderTextContent()}</span>
<div class={CSS.centerContent}>
<div class={CSS.title}>{this.renderTextContent(this.textLabel)}</div>
{this.description ? (
<div class={CSS.description}>{this.renderTextContent(this.description)}</div>
) : null}
</div>
{this.textLabelShort ? (
<div class={CSS.shortText}>{this.renderTextContent(this.textLabelShort)}</div>
) : null}
<slot name={SLOTS.contentEnd} />
</li>
{this.renderChildren()}
</div>
Expand All @@ -271,12 +295,14 @@ export class ComboboxItem implements ConditionalSlotComponent, InteractiveCompon
);
}

private renderTextContent(): string | (string | VNode)[] {
if (!this.filterTextMatchPattern) {
return this.textLabel;
private renderTextContent(text: string): string | (string | VNode)[] {
const pattern = this.filterTextMatchPattern;

if (!pattern || !text) {
return text;
}

const parts: (string | VNode)[] = this.textLabel.split(this.filterTextMatchPattern);
const parts: (string | VNode)[] = text.split(pattern);

if (parts.length > 1) {
// we only highlight the first match
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,24 @@
import { Scale } from "../interfaces";

export const CSS = {
icon: "icon",
iconActive: "icon--active",
iconIndent: "icon--indent",
active: "label--active",
centerContent: "center-content",
container: "container",
custom: "icon--custom",
description: "description",
dot: "icon--dot",
single: "label--single",
filterMatch: "filter-match",
icon: "icon",
iconActive: "icon--active",
label: "label",
active: "label--active",
scale: (scale: Scale) => `scale--${scale}` as const,
selected: "label--selected",
title: "title",
shortText: "short-text",
single: "label--single",
textContainer: "text-container",
filterMatch: "filter-match",
title: "title",
};

export const SLOTS = {
contentEnd: "content-end",
};
Loading
Loading