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

fix(combobox, stepper, table): respect user hidden attribute #10983

Merged
merged 31 commits into from
Jan 22, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6dfa7c9
fix(combobox, stepper, table): respect user hidden attribute
josercarcamo Dec 4, 2024
cddcabd
Merge branch 'dev' into josercarcamo/8623-respect-user-hidden-attribu…
josercarcamo Dec 12, 2024
ff38449
Merged dev
josercarcamo Jan 8, 2025
e843d98
Fixed all e2e tests for combobox
josercarcamo Jan 8, 2025
e425670
Fixed all tests for table
josercarcamo Jan 8, 2025
1c3680c
Removed unnecessary change
josercarcamo Jan 8, 2025
4772e23
Changed hideItem to hiddenItem and fixed test cases
josercarcamo Jan 9, 2025
46686b4
Finished addressing all code review comments
josercarcamo Jan 9, 2025
dfb66ae
Added hidden-item mixin
josercarcamo Jan 9, 2025
759791a
Removed unnecessary test change
josercarcamo Jan 10, 2025
eee513d
Merged dev
josercarcamo Jan 13, 2025
3e38b65
Merge branch 'dev' into josercarcamo/8623-respect-user-hidden-attribu…
josercarcamo Jan 13, 2025
284d75c
Merge branch 'dev' into josercarcamo/8623-respect-user-hidden-attribu…
josercarcamo Jan 13, 2025
09af985
Merge branch 'dev' into josercarcamo/8623-respect-user-hidden-attribu…
josercarcamo Jan 17, 2025
c88ca71
Used html formatter
josercarcamo Jan 17, 2025
a90211f
Parameterized disabled/hidden
josercarcamo Jan 17, 2025
7d1f446
Changed step number to integer
josercarcamo Jan 17, 2025
f8a9016
Removed optional chaining operator; fixed combobox e2e error
josercarcamo Jan 17, 2025
37cabf8
Refactored statement in test per code review
josercarcamo Jan 18, 2025
3e146b9
Removed unnecessary test per review
josercarcamo Jan 18, 2025
1d4a2d3
Renamed argument to el
josercarcamo Jan 18, 2025
a46227d
Replaced conditional with use of isHidden
josercarcamo Jan 18, 2025
da25db2
Changed test to step through hidden element
josercarcamo Jan 18, 2025
ca3fedc
Fixed combobox test failure
josercarcamo Jan 20, 2025
a3ea10a
Fixed errors caused by hidden combobox item'
josercarcamo Jan 20, 2025
7da4daa
Merge branch 'dev' into josercarcamo/8623-respect-user-hidden-attribu…
josercarcamo Jan 20, 2025
4f6d67a
Changed wording of comment and name of argument
josercarcamo Jan 21, 2025
e07beac
Merge branch 'josercarcamo/8623-respect-user-hidden-attribute-v2' of …
josercarcamo Jan 21, 2025
8c8c883
Renamed 'hiddenItem' to 'calciteHidden'
josercarcamo Jan 22, 2025
d867b85
Renamed 'calciteHidden' to 'itemHidden'
josercarcamo Jan 22, 2025
26f24a1
Merge branch 'dev' into josercarcamo/8623-respect-user-hidden-attribu…
josercarcamo Jan 22, 2025
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
6 changes: 2 additions & 4 deletions packages/calcite-components/src/assets/styles/includes.scss
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,8 @@
}

@mixin base-component() {
:host([hidden]) {
Copy link
Member

Choose a reason for hiding this comment

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

Since the internal hide-item attribute is only applicable to certain components, can you create a separate mixin and include in applicable children?

@apply hidden;
}

:host([hidden]),
:host([hide-item]),
[hidden] {
@apply hidden;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ export class ComboboxItem extends LitElement implements InteractiveComponent {
*/
@property() value: any;

/**
* Specifies whether the user set the hidden attribute in the HTML
*
*/
@property() hideItem = false;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for removing user from the prop name! To align with hidden, WDYT about hiddenItem?


// #endregion

// #region Events
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,56 @@ describe("calcite-combobox", () => {
}
});

it("should show correct number of items when child hidden", async () => {
const page = await newE2EPage();
await page.setContent(`
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the html formatting helper, so prettier formats embedded HTML? Applies to other test page markup.

<calcite-combobox label="custom values" allow-custom-values placeholder="placeholder" max-items="6">
<calcite-combobox-item value="Trees" text-label="Trees" selected>
<calcite-combobox-item value="Pine" text-label="Pine">
<calcite-combobox-item value="Pine Nested" text-label="Pine Nested"></calcite-combobox-item>
</calcite-combobox-item>
<calcite-combobox-item value="Sequoia" hide-item text-label="Sequoia"></calcite-combobox-item>
<calcite-combobox-item value="Douglas Fir" text-label="Douglas Fir"></calcite-combobox-item>
</calcite-combobox-item>
<calcite-combobox-item value="Rocks" text-label="Rocks"></calcite-combobox-item>
</calcite-combobox>
`);
await page.waitForChanges();

const element = await page.find("calcite-combobox");
await element.click();
await page.waitForChanges();
const items = await page.findAll("calcite-combobox-item, calcite-combobox-item-group");
for (let i = 0; i < items.length; i++) {
expect(await items[i].isIntersectingViewport()).toBe(i !== 3);
}
});

it("should show correct number of items when parent hidden", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-combobox label="custom values" allow-custom-values placeholder="placeholder" max-items="6">
<calcite-combobox-item value="Trees" text-label="Trees" hide-item>
<calcite-combobox-item value="Pine" text-label="Pine">
<calcite-combobox-item value="Pine Nested" text-label="Pine Nested"></calcite-combobox-item>
</calcite-combobox-item>
<calcite-combobox-item value="Sequoia" disabled text-label="Sequoia"></calcite-combobox-item>
<calcite-combobox-item value="Douglas Fir" text-label="Douglas Fir"></calcite-combobox-item>
</calcite-combobox-item>
<calcite-combobox-item value="Rocks" text-label="Rocks"></calcite-combobox-item>
</calcite-combobox>
`);
await page.waitForChanges();

const element = await page.find("calcite-combobox");
await element.click();
await page.waitForChanges();
const items = await page.findAll("calcite-combobox-item, calcite-combobox-item-group");
for (let i = 0; i < items.length; i++) {
expect(await items[i].isIntersectingViewport()).toBe(i === 5);
}
});

it("should show correct max items after selection", async () => {
const page = await newE2EPage();
const maxItems = 6;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1299,7 +1299,7 @@ export class Combobox
const items: HTMLCalciteComboboxItemElement["el"][] = Array.from(
this.el.querySelectorAll(ComboboxItemSelector),
);
return items.filter((item) => !item.disabled);
return items.filter((item) => !item.disabled && !item.hideItem);
}

private getGroupItems(): HTMLCalciteComboboxItemGroupElement["el"][] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ export class StepperItem extends LitElement implements InteractiveComponent, Loa
/** When `true`, the icon will be flipped when the element direction is right-to-left (`"rtl"`). */
@property({ reflect: true }) iconFlipRtl = false;

/** When `true`, the item will be hidden */
@property({ reflect: true }) hideItem = false;

/**
* Specifies the layout of the `calcite-stepper-item` inherited from parent `calcite-stepper`, defaults to `horizontal`.
*
Expand Down Expand Up @@ -273,6 +276,7 @@ export class StepperItem extends LitElement implements InteractiveComponent, Loa
private handleItemClick(event: MouseEvent): void {
if (
this.disabled ||
this.hideItem ||
(this.layout === "horizontal" &&
event
.composedPath()
Expand Down Expand Up @@ -302,9 +306,9 @@ export class StepperItem extends LitElement implements InteractiveComponent, Loa
}

private getItemPosition(): number {
return Array.from(this.parentStepperEl?.querySelectorAll("calcite-stepper-item")).indexOf(
this.el,
);
return Array.from(this.parentStepperEl?.querySelectorAll("calcite-stepper-item"))
.filter((s) => !s.hidden && !s.hideItem)
.indexOf(this.el);
}

// #endregion
Expand Down
71 changes: 54 additions & 17 deletions packages/calcite-components/src/components/stepper/stepper.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ describe("calcite-stepper", () => {
expect(await step4Content.isVisible()).toBe(false);
});

it("navigates disabled items correctly with nextStep and prevStep methods", async () => {
it("navigates disabled and hidden items correctly with nextStep and prevStep methods", async () => {
const page = await newE2EPage();
await page.setContent(
html`<calcite-stepper>
Expand All @@ -262,37 +262,48 @@ describe("calcite-stepper", () => {
<calcite-stepper-item heading="Step 3" id="step-3">
<div>Step 3 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 4" id="step-4" hide-item>
Copy link
Member

Choose a reason for hiding this comment

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

FIXME: hide-item is internal, so it shouldn't be used in tests. You'll need to update lines like the following to use the internal prop instead. This change will ensure users can hide elements without our components making them unintentionally visible.

<div>Step 4 content</div>
</calcite-stepper-item>
</calcite-stepper>`,
);
const element = await page.find("calcite-stepper");
const step1 = await page.find("#step-1");
const step2 = await page.find("#step-2");
const step3 = await page.find("#step-3");
const step4 = await page.find("#step-4");
const step1Content = await page.find("#step-1 >>> .stepper-item-content");
const step2Content = await page.find("#step-2 >>> .stepper-item-content");
const step3Content = await page.find("#step-3 >>> .stepper-item-content");
const step4Content = await page.find("#step-4 >>> .stepper-item-content");
expect(step1).toHaveAttribute("selected");
expect(step2).not.toHaveAttribute("selected");
expect(step3).not.toHaveAttribute("selected");
expect(step4).not.toHaveAttribute("selected");
expect(await step1Content.isVisible()).toBe(true);
expect(await step2Content.isVisible()).toBe(false);
expect(await step3Content.isVisible()).toBe(false);
expect(await step4Content.isVisible()).toBe(false);
await element.callMethod("nextStep");
await page.waitForChanges();
expect(step1).not.toHaveAttribute("selected");
expect(step2).not.toHaveAttribute("selected");
expect(step3).toHaveAttribute("selected");
expect(step4).not.toHaveAttribute("selected");
expect(await step1Content.isVisible()).toBe(false);
expect(await step2Content.isVisible()).toBe(false);
expect(await step3Content.isVisible()).toBe(true);
expect(await step4Content.isVisible()).toBe(false);
await element.callMethod("prevStep");
await page.waitForChanges();
expect(step1).toHaveAttribute("selected");
expect(step2).not.toHaveAttribute("selected");
expect(step3).not.toHaveAttribute("selected");
expect(step4).not.toHaveAttribute("selected");
jcfranco marked this conversation as resolved.
Show resolved Hide resolved
expect(await step1Content.isVisible()).toBe(true);
expect(await step2Content.isVisible()).toBe(false);
expect(await step3Content.isVisible()).toBe(false);
expect(await step4Content.isVisible()).toBe(false);
});

it("navigates correctly with startStep and endStep methods", async () => {
Expand Down Expand Up @@ -344,7 +355,7 @@ describe("calcite-stepper", () => {
expect(await step4Content.isVisible()).toBe(true);
});

it("navigates disabled items correctly with startStep and endStep methods", async () => {
it("navigates disabled and hidden items correctly with startStep and endStep methods", async () => {
const page = await newE2EPage();
await page.setContent(
html`<calcite-stepper>
Expand All @@ -360,37 +371,46 @@ describe("calcite-stepper", () => {
<calcite-stepper-item heading="Step 4" id="step-4" disabled>
<div>Step 4 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 5" id="step-5" hide-item>
<div>Step 5 content</div>
</calcite-stepper-item>
</calcite-stepper>`,
);
const element = await page.find("calcite-stepper");
const step1 = await page.find("#step-1");
const step2 = await page.find("#step-2");
const step3 = await page.find("#step-3");
const step4 = await page.find("#step-4");
const step5 = await page.find("#step-5");
const step1Content = await page.find("#step-1 >>> .stepper-item-content");
const step2Content = await page.find("#step-2 >>> .stepper-item-content");
const step3Content = await page.find("#step-3 >>> .stepper-item-content");
const step4Content = await page.find("#step-4 >>> .stepper-item-content");
const step5Content = await page.find("#step-5 >>> .stepper-item-content");
await element.callMethod("endStep");
await page.waitForChanges();
expect(step1).not.toHaveAttribute("selected");
expect(step2).not.toHaveAttribute("selected");
expect(step3).toHaveAttribute("selected");
expect(step4).not.toHaveAttribute("selected");
expect(step5).not.toHaveAttribute("selected");
expect(await step1Content.isVisible()).toBe(false);
expect(await step2Content.isVisible()).toBe(false);
expect(await step3Content.isVisible()).toBe(true);
expect(await step4Content.isVisible()).toBe(false);
expect(await step5Content.isVisible()).toBe(false);
await element.callMethod("startStep");
await page.waitForChanges();
expect(step1).not.toHaveAttribute("selected");
expect(step2).toHaveAttribute("selected");
expect(step3).not.toHaveAttribute("selected");
expect(step4).not.toHaveAttribute("selected");
expect(step5).not.toHaveAttribute("selected");
expect(await step1Content.isVisible()).toBe(false);
expect(await step2Content.isVisible()).toBe(true);
expect(await step3Content.isVisible()).toBe(false);
expect(await step4Content.isVisible()).toBe(false);
expect(await step5Content.isVisible()).toBe(false);
});

it("navigates to requested step with goToStep method", async () => {
Expand Down Expand Up @@ -560,6 +580,10 @@ describe("calcite-stepper", () => {
expect(changeSpy).toHaveReceivedEventTimes(expectedEvents);
expect(await getSelectedItemId()).toBe("step-4");

await page.$eval("#step-5", itemClicker);
expect(itemChangeSpy).toHaveReceivedEventTimes(expectedEvents);
expect(changeSpy).toHaveReceivedEventTimes(expectedEvents);

await element.callMethod("prevStep");
await page.waitForChanges();
expect(itemChangeSpy).toHaveReceivedEventTimes(expectedEvents);
Expand Down Expand Up @@ -592,6 +616,9 @@ describe("calcite-stepper", () => {
<calcite-stepper-item heading="Step 4" id="step-4">
<div>Step 4 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 5" id="step-5" hide-item>
<div>Step 5 content</div>
</calcite-stepper-item>
</calcite-stepper>`,
);

Expand All @@ -606,6 +633,7 @@ describe("calcite-stepper", () => {
<calcite-stepper-item heading="Step 2" id="step-2"></calcite-stepper-item>
<calcite-stepper-item heading="Step 3" id="step-3" disabled></calcite-stepper-item>
<calcite-stepper-item heading="Step 4" id="step-4"></calcite-stepper-item>
<calcite-stepper-item heading="Step 5" id="step-5" hide-item></calcite-stepper-item>
</calcite-stepper>`,
);

Expand Down Expand Up @@ -634,6 +662,9 @@ describe("calcite-stepper", () => {
<calcite-stepper-item heading="Step 4" id="step-4">
<div>Step 4 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 5" id="step-5" hide-item>
<div>Step 5 content</div>
</calcite-stepper-item>
</calcite-stepper>`,
);

Expand All @@ -648,6 +679,7 @@ describe("calcite-stepper", () => {
<calcite-stepper-item heading="Step 2" id="step-2"></calcite-stepper-item>
<calcite-stepper-item heading="Step 3" id="step-3" disabled></calcite-stepper-item>
<calcite-stepper-item heading="Step 4" id="step-4"></calcite-stepper-item>
<calcite-stepper-item heading="Step 5" id="step-5" hide-item></calcite-stepper-item>
</calcite-stepper>`,
);

Expand Down Expand Up @@ -739,22 +771,24 @@ describe("calcite-stepper", () => {
expect(await stepperItem3.getProperty("selected")).not.toBe(true);
});

it("should select the next enabled stepper-item if first stepper-item is disabled", async () => {
const page = await newE2EPage();
await page.setContent(
html`<calcite-stepper>
<calcite-stepper-item heading="Step 1" id="step-1" disabled>
<div>Step 1 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 2" id="step-2">
<div>Step 2 content</div>
</calcite-stepper-item>
</calcite-stepper>`,
);
it("should select the next enabled stepper-item if first stepper-item is disabled or hidden", async () => {
for (const n of ["disabled", "hidden"]) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use it.each for this instead.

const page = await newE2EPage();
await page.setContent(
html`<calcite-stepper>
<calcite-stepper-item heading="Step 1" id="step-1" ${n}>
<div>Step 1 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 2" id="step-2">
<div>Step 2 content</div>
</calcite-stepper-item>
</calcite-stepper>`,
);

const [stepperItem1, stepperItem2] = await page.findAll("calcite-stepper-item");
expect(await stepperItem1.getProperty("selected")).toBe(false);
expect(await stepperItem2.getProperty("selected")).toBe(true);
const [stepperItem1, stepperItem2] = await page.findAll("calcite-stepper-item");
expect(await stepperItem1.getProperty("selected")).toBe(false);
expect(await stepperItem2.getProperty("selected")).toBe(true);
}
});

describe("horizontal-single layout", () => {
Expand Down Expand Up @@ -843,6 +877,9 @@ describe("calcite-stepper", () => {
<calcite-stepper-item heading="Step 2" id="step-2" disabled>
<div>Step 2 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 2.1" id="step-2.1" hide-item>
<div>Step 2.1 content</div>
</calcite-stepper-item>
<calcite-stepper-item heading="Step 3" id="step-2">
<div>Step 3 content</div>
</calcite-stepper-item>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ export class Stepper extends LitElement {
}

private filterItems(): StepperItem["el"][] {
return this.items.filter((item) => !item.disabled);
return this.items.filter((item) => !item.disabled && !item.hideItem);
}

private setStepperItemNumberingSystem(): void {
Expand Down Expand Up @@ -389,7 +389,8 @@ export class Stepper extends LitElement {

private handleDefaultSlotChange(event: Event): void {
const items = slotChangeGetAssignedElements(event).filter(
(el): el is StepperItem["el"] => el?.tagName === "CALCITE-STEPPER-ITEM",
(el): el is StepperItem["el"] =>
el?.tagName === "CALCITE-STEPPER-ITEM" && !(el as StepperItem["el"])?.hideItem,
);
this.items = items;
const spacing = Array(items.length).fill("1fr").join(" ");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ export class TableRow extends LitElement implements InteractiveComponent {
/** Specifies the alignment of the component. */
@property({ reflect: true }) alignment: Alignment;

/** Specifies whether this row should be hidden */
@property() hideItem = false;

/** @private */
@property() bodyRowCount: number;

Expand Down
Loading
Loading