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(value-list-item): add new slot labelled as content-center to replace label/description (#3373) #3579

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
97b6fa6
style: minimize title/description space on the list when prop value i…
Nov 29, 2021
b7b7d6b
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Nov 29, 2021
5234a28
add new slot labelled as content-center to replace label/description
Dec 6, 2021
2b620d2
add styling for content-center slot in default and non interactive state
Dec 6, 2021
3260150
render content center that takes precedence over label/description an…
Dec 6, 2021
592e7e2
add padding for content-center slot
Dec 6, 2021
77a25a3
add story for new slot enhancement
Dec 6, 2021
bcc9914
Merge branch 'y0n4/3373-value-list-label' of github.com:Esri/calcite-…
Dec 6, 2021
8b050b1
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 6, 2021
9adb284
keep the label/description the way it was originally
Dec 7, 2021
45dea00
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 7, 2021
7d01e5b
add documentation for slot content-center
Dec 7, 2021
c88bb11
change variable name and reword the prop documentation
Dec 8, 2021
046b914
add padding to slot content
Dec 13, 2021
cc70156
prevent focus if user clicks on any content inside of the slot only
Dec 13, 2021
c828c27
change change slot name from content-center to just content
Dec 13, 2021
731edd8
add slot custom content with dynamic content
Dec 13, 2021
14069d8
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 13, 2021
040d3ce
remove local testing component change
Dec 15, 2021
3e516e6
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 15, 2021
0f21fcb
adjust for pr review comments
Dec 16, 2021
f6d69f7
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 16, 2021
7a24d68
remove extra focusDiv state to be shared pr commentr
Dec 20, 2021
795e018
add absolute positioning to prevent focus event from bubbling up
Dec 20, 2021
b27ce13
remove stop propagation and shift the content slot above the select c…
Dec 20, 2021
4083612
remove duplicate styling classes
Dec 20, 2021
c519f56
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 20, 2021
5f39db5
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 21, 2021
7526d00
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 22, 2021
5db919d
swap the absolute positioning between the content div child
Dec 27, 2021
71881c7
Merge branch 'master' of github.com:Esri/calcite-components into y0n4…
Dec 28, 2021
2bec25e
change back to previous tailwind class for selected item since new cl…
Dec 29, 2021
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
51 changes: 47 additions & 4 deletions src/components/calcite-pick-list-item/calcite-pick-list-item.scss
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@
.label {
@apply bg-transparent
flex
flex-auto
items-center
cursor-pointer
focus-base;
focus-base
w-full;
&:focus {
@apply focus-inset;
}
Expand All @@ -38,7 +38,8 @@
}

:host([non-interactive]) .label,
:host([non-interactive]) .icon {
:host([non-interactive]) .icon,
:host([non-interactive]) .content--select {
@apply pointer-events-none;
}

Expand Down Expand Up @@ -91,9 +92,51 @@
}

.actions {
@apply items-stretch flex justify-end m-0 flex-initial;
@apply items-stretch flex m-0 flex-initial;
}

.actions--start ~ .label {
padding-inline-start: theme("padding.1");
}

.actions--end {
@apply justify-end flex-1;
}

.content {
@apply w-full
h-full
relative
flex
items-center;
}

.content--select {
@apply bg-transparent
cursor-pointer
focus-base
w-full
h-full
absolute;
Copy link
Member

Choose a reason for hiding this comment

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

@asangma @y0n4 why is this positioned absolutely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did absolute position for the selected portion of the item in relative to the current content. That way users can have the choice to focus on the item itself or the content within. This is to escape the events from bubbling up and an alternative to the stopPropagation method.

&:focus {
@apply focus-inset;
}
&:hover {
@apply bg-foreground-2;
}
}

.content--slot {
@apply py-2
px-3;
}

:host([non-interactive]) .content {
@apply cursor-default;
&:focus {
@apply focus-base;
}
&:hover {
@apply bg-transparent;
}
}
63 changes: 43 additions & 20 deletions src/components/calcite-pick-list-item/calcite-pick-list-item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { getSlotted } from "../../utils/dom";
/**
* @slot actions-end - a slot for adding actions or content to the end side of the item.
* @slot actions-start - a slot for adding actions or content to the start side of the item.
* @slot content - a slot for adding content at the center of the item and it will take precedence over label prop if utilized.
*/
@Component({
tag: "calcite-pick-list-item",
Expand Down Expand Up @@ -127,7 +128,7 @@ export class CalcitePickListItem {

@Element() el: HTMLCalcitePickListItemElement;

private focusEl: HTMLLabelElement;
private focusEl: HTMLLabelElement | HTMLDivElement;

shiftPressed: boolean;

Expand Down Expand Up @@ -270,6 +271,46 @@ export class CalcitePickListItem {
) : null;
}

renderContent(): VNode {
const { description, label, el } = this;
const hasContentSlot = getSlotted(el, SLOTS.content);

return hasContentSlot ? (
<div class={CSS.content}>
<div
aria-checked={this.selected.toString()}
Copy link
Member

Choose a reason for hiding this comment

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

this checkbox role element is missing a label. Can you verify this passes aXe? Can we add an updated accessible e2e test with this slot?

Copy link
Member

Choose a reason for hiding this comment

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

Can we just keep the existing label element and just replace the following HTML with the slot?

<span class={CSS.title}>{label}</span>
          {description ? <span class={CSS.description}>{description}</span> : null}

class={CSS.contentSelect}
onClick={this.pickListClickHandler}
onKeyDown={this.pickListKeyDownHandler}
ref={(focusEl): HTMLDivElement => (this.focusEl = focusEl)}
role="menuitemcheckbox"
tabIndex={0}
/>
<div class={CSS.contentSlot}>
<slot name={SLOTS.content} />
</div>
</div>
) : (
<label
aria-label={label}
class={CSS.label}
onClick={this.pickListClickHandler}
onKeyDown={this.pickListKeyDownHandler}
ref={(focusEl): HTMLLabelElement => (this.focusEl = focusEl)}
tabIndex={0}
>
<div
aria-checked={this.selected.toString()}
y0n4 marked this conversation as resolved.
Show resolved Hide resolved
class={CSS.textContainer}
role="menuitemcheckbox"
>
<span class={CSS.title}>{label}</span>
{description ? <span class={CSS.description}>{description}</span> : null}
</div>
</label>
);
}

renderActionsEnd(): VNode {
const { el, removable } = this;
const hasActionsEnd = getSlotted(el, SLOTS.actionsEnd);
Expand All @@ -283,29 +324,11 @@ export class CalcitePickListItem {
}

render(): VNode {
const { description, label } = this;

return (
<Fragment>
{this.renderIcon()}
{this.renderActionsStart()}
<label
aria-label={label}
class={CSS.label}
onClick={this.pickListClickHandler}
onKeyDown={this.pickListKeyDownHandler}
ref={(focusEl): HTMLLabelElement => (this.focusEl = focusEl)}
tabIndex={0}
>
<div
aria-checked={this.selected.toString()}
class={CSS.textContainer}
role="menuitemcheckbox"
>
<span class={CSS.title}>{label}</span>
{description ? <span class={CSS.description}>{description}</span> : null}
</div>
</label>
{this.renderContent()}
{this.renderActionsEnd()}
</Fragment>
);
Expand Down
4 changes: 4 additions & 0 deletions src/components/calcite-pick-list-item/resources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ export const CSS = {
actions: "actions",
actionsEnd: "actions--end",
actionsStart: "actions--start",
content: "content",
contentSelect: "content--select",
contentSlot: "content--slot",
description: "description",
handle: "handle",
handleActivated: "handle--activated",
Expand All @@ -20,6 +23,7 @@ export const ICONS = {
};

export const SLOTS = {
content: "content",
actionsEnd: "actions-end",
actionsStart: "actions-start"
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ calcite-pick-list-item {

:host([active]),
:host([selected]) {
@apply ring;
@apply shadow-outline-active;
}

.handle {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { getSlotted } from "../../utils/dom";
/**
* @slot actions-end - A slot for adding actions or content to the end side of the item.
* @slot actions-start - A slot for adding actions or content to the start side of the item.
* @slot content - a slot for adding content at the center of the item and it will take precedence over label prop if utilized.
*/
@Component({
tag: "calcite-value-list-item",
Expand Down Expand Up @@ -67,7 +68,7 @@ export class CalciteValueListItem {
/**
* The main label for this item. Appears next to the icon.
*/
@Prop({ reflect: true }) label!: string;
@Prop({ reflect: true }) label: string;

/**
* Used to provide additional metadata to an item, primarily used when the parent list has a filter.
Expand Down Expand Up @@ -186,6 +187,13 @@ export class CalciteValueListItem {
) : null;
}

renderContent(): VNode {
const { el } = this;
const hasContentSlot = getSlotted(el, SLOTS.content);

return hasContentSlot ? <slot name={SLOTS.content} slot={PICK_LIST_SLOTS.content} /> : null;
}

renderHandle(): VNode {
const { icon } = this;
if (icon === ICON_TYPES.grip) {
Expand Down Expand Up @@ -226,6 +234,7 @@ export class CalciteValueListItem {
value={this.value}
>
{this.renderActionsStart()}
{this.renderContent()}
{this.renderActionsEnd()}
</calcite-pick-list-item>
</Host>
Expand Down
3 changes: 2 additions & 1 deletion src/components/calcite-value-list-item/resources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ export const ICONS = {

export const SLOTS = {
actionsEnd: "actions-end",
actionsStart: "actions-start"
actionsStart: "actions-start",
content: "content"
};
17 changes: 17 additions & 0 deletions src/components/calcite-value-list/calcite-value-list.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,23 @@ export const basic = (): string =>
`
);

export const withContentSlot = (): string =>
create(
"calcite-value-list",
createAttributes(),
html`
<calcite-value-list-item label="Default label and description" description="used as content" value="dogs">
${action}
</calcite-value-list-item>
<calcite-value-list-item value="cats">
<div slot="content">
<calcite-input placeholder="slot content"></calcite-input>
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 this will be problematic, the role of the list-item is a checkbox. The checkbox can't contain an input inside of it I don't think. The DOM structure and roles would need to be reconfigured.

Can we make sure that this HTML passes aXe?

</div>
${action}
</calcite-value-list-item>
`
);

export const darkThemeRTL = (): string =>
create(
"calcite-value-list",
Expand Down