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.

Sign up for GitHub

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(list-item): Add calciteListItemToggle event. #8433

Merged
merged 9 commits into from
Dec 16, 2023
Merged

feat(list-item): Add calciteListItemToggle event. #8433

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5ffcb06
feat(list-item): Add calciteListItemOpen event.
driskull Dec 15, 2023
439cb66
rename event to calciteListItemToggle
driskull Dec 15, 2023
db84875
rename to calciteListItemOpenToggle
driskull Dec 15, 2023
b512235
rename event to calciteListItemOpenToggle
driskull Dec 15, 2023
56f328c
update components.d.ts
driskull Dec 16, 2023
bd40bdf
Merge branch 'main' into dris0000/list-item-open-event
driskull Dec 16, 2023
4d35e8b
make private methods private
driskull Dec 16, 2023
5207077
review fixes
driskull Dec 16, 2023
4c796fa
Merge branch 'dris0000/list-item-open-event' of github.com:Esri/calci…
driskull Dec 16, 2023
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
5 changes: 5 additions & 0 deletions packages/calcite-components/src/components.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6286,6 +6286,7 @@ declare global {
interface HTMLCalciteListItemElementEventMap {
"calciteListItemSelect": void;
"calciteListItemClose": void;
"calciteListItemOpen": void;
"calciteInternalListItemSelect": void;
"calciteInternalListItemSelectMultiple": {
selectMultiple: boolean;
Expand Down Expand Up @@ -10079,6 +10080,10 @@ declare namespace LocalJSX {
* Fires when the close button is clicked.
*/
"onCalciteListItemClose"?: (event: CalciteListItemCustomEvent<void>) => void;
/**
* Fires when the open button is clicked.
*/
"onCalciteListItemOpen"?: (event: CalciteListItemCustomEvent<void>) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly because we already have ‘close’ event for the “close button” interaction, but is it possible to use Toggle here or something besides ‘open’? We use ‘calcitePanelToggle‘ in Panel. It could also fire when collapsed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I can do that. its mapped to the open property though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it’s tough with existing ‘close’ as that, in practice, is more of “dismiss” - is Toggle is too close to ‘select’ event? OpenToggle?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. Ill let @jcfranco comment on name as well.

Copy link
Member

Choose a reason for hiding this comment

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

TL;DR: I think we should go with calciteListItemToggle event firing when opened/closed.

I'd like to avoid introducing an event name that mixes both open/close and toggling. We could introduce an individual toggle event (similar to block's toggle event before we added open/close events) and check the event target's open value for now until we look more into the following:

Unfortunately, list is in a bit of a tough spot with some of these props and patterns we're adopting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks 👍

/**
* Emits when the item's content is selected.
*/
Expand Down
16 changes: 16 additions & 0 deletions packages/calcite-components/src/components/list-item/list-item.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,4 +275,20 @@ describe("calcite-list-item", () => {

expect(calciteListItemClose).toHaveReceivedEventTimes(1);
});

it("should fire open event when opened", async () => {
const page = await newE2EPage({
html: html`<calcite-list-item
><calcite-list><calcite-list-item></calcite-list-item></calcite-list
></calcite-list-item>`,
});

const calciteListItemOpen = await page.spyOnEvent("calciteListItemOpen", "window");

const openButton = await page.find(`calcite-list-item >>> .${CSS.openContainer}`);

await openButton.click();

expect(calciteListItemOpen).toHaveReceivedEventTimes(1);
});
});
30 changes: 20 additions & 10 deletions packages/calcite-components/src/components/list-item/list-item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,11 @@ export class ListItem
*/
@Event({ cancelable: false }) calciteListItemClose: EventEmitter<void>;

/**
* Fires when the open button is clicked.
*/
@Event({ cancelable: false }) calciteListItemOpen: EventEmitter<void>;

/**
*
* @internal
Expand Down Expand Up @@ -397,7 +402,7 @@ export class ListItem
}

return (
<td class={CSS.selectionContainer} key="selection-container" onClick={this.itemClicked}>
<td class={CSS.selectionContainer} key="selection-container" onClick={this.handleItemClick}>
<calcite-icon
icon={
selected
Expand Down Expand Up @@ -446,7 +451,7 @@ export class ListItem
: ICONS.closedLTR
: ICONS.blank;

const clickHandler = openable ? this.toggleOpen : this.itemClicked;
const clickHandler = openable ? this.handleToggleOpenClick : this.handleItemClick;

return openable || parentListEl?.openable ? (
<td class={CSS.openContainer} key="open-container" onClick={clickHandler}>
Expand Down Expand Up @@ -491,7 +496,7 @@ export class ListItem
icon={ICONS.close}
key="close-action"
label={messages.close}
onClick={this.closeClickHandler}
onClick={this.handleCloseClick}
text={messages.close}
/>
) : null}
Expand Down Expand Up @@ -593,7 +598,7 @@ export class ListItem
[CSS.contentContainerHasCenterContent]: hasCenterContent,
}}
key="content-container"
onClick={this.itemClicked}
onClick={this.handleItemClick}
role="gridcell"
// eslint-disable-next-line react/jsx-sort-props -- ref should be last so node attrs/props are in sync (see https://github.com/Esri/calcite-design-system/pull/6530)
ref={(el) => (this.contentEl = el)}
Expand Down Expand Up @@ -669,7 +674,7 @@ export class ListItem
this.calciteInternalListItemChange.emit();
}

closeClickHandler = (): void => {
handleCloseClick = (): void => {
this.closed = true;
this.calciteListItemClose.emit();
};
Expand Down Expand Up @@ -740,11 +745,16 @@ export class ListItem
this.handleOpenableChange(event.target as HTMLSlotElement);
};

toggleOpen = (): void => {
this.open = !this.open;
handleToggleOpenClick = (): void => {
this.toggleOpen();
};

toggleOpen = (value = !this.open): void => {
this.open = value;
this.calciteListItemOpen.emit();
};

itemClicked = (event: PointerEvent): void => {
handleItemClick = (event: PointerEvent): void => {
if (event.defaultPrevented) {
return;
}
Expand Down Expand Up @@ -802,7 +812,7 @@ export class ListItem
const nextIndex = currentIndex + 1;
if (currentIndex === -1) {
if (!open && openable) {
this.open = true;
this.toggleOpen(true);
this.focusCell(null);
} else if (cells[0]) {
this.focusCell(cells[0]);
Expand All @@ -816,7 +826,7 @@ export class ListItem
if (currentIndex === -1) {
this.focusCell(null);
if (open && openable) {
this.open = false;
this.toggleOpen(false);
} else {
this.calciteInternalFocusPreviousItem.emit();
}
Expand Down