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 26 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
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ Calcite follows [Conventional Commits](https://www.conventionalcommits.org/en/v1
Contributions should adhere to the `<type>(<scope>): <descriptive summary>` format and include the following:

- [Commit type](#commit-type)
- [Scope of change](#scope-of-change), _optional_
- [Scope of change](#scope-of-change), *optional*
- [Descriptive commit subject](#descriptive-commit-subject)

Check out the [contribution example](#contribution-example) for a formatted example, and explore [breaking change formatting](#breaking-changes) for consideration during Calcite's breaking change releases.
Expand All @@ -240,7 +240,7 @@ Contributions must adhere to **one** of the following types:

### Scope of change

_Optional_. Most contributions will include a scope, such as a component, multiple components, test(s), or utilities. For example:
*Optional*. Most contributions will include a scope, such as a component, multiple components, test(s), or utilities. For example:

- `text-area`
- `dropdown, dropdown-group, dropdown-item`
Expand Down
6 changes: 6 additions & 0 deletions packages/calcite-components/src/assets/styles/includes.scss
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ $common-animatable-props: "background-color, block-size, border-color, box-shado
transition-timing-function: ease-in-out;
}

@mixin hidden-item() {
:host([hidden-item]) {
@apply hidden;
}
}

// Mixin for text highlighting styles.
// - this should be used in conjunction with the `text.tsx` util.
@mixin text-highlight-item() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,4 @@
}

@include base-component();
@include hidden-item();
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ export class ComboboxItemGroup extends LitElement {
*/
@property() scale: Scale = "m";

/**
* Specifies whether the user set the hidden attribute in the HTML
Copy link
Member

Choose a reason for hiding this comment

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

This comment doesn't seem to be accurate. Can you give it another look? Applies to most hiddenItem prop doc.

*
* @private
*/
@property({ reflect: true }) hiddenItem = false;
Copy link
Member

Choose a reason for hiding this comment

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

Should we have e2e defaults and reflects tests for each of these new private properties?

Copy link
Member

Choose a reason for hiding this comment

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

Not for internal props, but the controller approach could help us test this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Not for internal props, but the controller approach could help us test this behavior.

It would be beneficial even for internal props because if this property isn't reflected then the CSS and querySelector logic won't work. I would think it would be beneficial

Copy link
Member

Choose a reason for hiding this comment

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

That should be handled by the controller testing.


// #endregion

// #region Lifecycle
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,5 @@ ul:focus {
line-height: var(--calcite-font-line-height-relative-snug);
}

@include hidden-item();
@include text-highlight-item();
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,13 @@ export class ComboboxItem extends LitElement implements InteractiveComponent {
*/
@property() value: any;

/**
* Specifies whether the user set the hidden attribute in the HTML
*
* @private
*/
@property({ reflect: true }) hiddenItem = false;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: would itemHidden be preferred here? it aligns better with our other props like feature-disabled.

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 had proposed that but it was decided to go with hiddenItem.

Copy link
Member

Choose a reason for hiding this comment

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

@jcfranco what you think? 🤔 It doesn't matter much since its a private property but would align better with other props as itemHidden or even calciteHidden to illustrate that calcite is hiding this.

Copy link
Member

Choose a reason for hiding this comment

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

Renaming sounds good for consistency. @josercarcamo I'll defer to you whether you want to tackle that here or separately.

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'll tackle it here. I'll change it to calciteHidden, if there are no objections.


// #endregion

// #region Events
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ describe("calcite-combobox", () => {
await page.waitForTimeout(DEBOUNCE.filter);

const visibleItemsAndGroups = await page.findAll(
"calcite-combobox-item:not([hidden]), calcite-combobox-item-group:not([hidden])",
"calcite-combobox-item:not([hidden-item]), calcite-combobox-item-group:not([hidden-item])",
);
const visibleItemAndGroupIds = await Promise.all(visibleItemsAndGroups.map((item) => item.getProperty("id")));

Expand Down Expand Up @@ -497,7 +497,7 @@ describe("calcite-combobox", () => {
await page.waitForTimeout(DEBOUNCE.filter);

const filteredItemsAndGroups = await page.findAll(
"calcite-combobox-item:not([hidden]), calcite-combobox-item-group:not([hidden])",
"calcite-combobox-item:not([hidden-item]), calcite-combobox-item-group:not([hidden-item])",
);
const filteredItemAndGroupIds = await Promise.all(filteredItemsAndGroups.map((item) => item.getProperty("id")));

Expand All @@ -518,7 +518,7 @@ describe("calcite-combobox", () => {
await page.waitForTimeout(DEBOUNCE.filter);

const allVisibleItemAndGroups = await page.findAll(
"calcite-combobox-item:not([hidden]), calcite-combobox-item-group:not([hidden])",
"calcite-combobox-item:not([hidden]):not([hidden-item]), calcite-combobox-item-group:not([hidden]):not([hidden-item])",
);
const allVisibleItemAndGroupIds = await Promise.all(
allVisibleItemAndGroups.map((item) => item.getProperty("id")),
Expand Down Expand Up @@ -570,7 +570,7 @@ describe("calcite-combobox", () => {
await page.waitForChanges();
await page.waitForTimeout(DEBOUNCE.filter);

const visibleItems = await page.findAll("calcite-combobox-item:not([hidden])");
const visibleItems = await page.findAll("calcite-combobox-item:not([hidden-item])");

expect(visibleItems.length).toBe(1);
expect(await visibleItems[0].getProperty("value")).toBe("1");
Expand Down Expand Up @@ -725,7 +725,7 @@ describe("calcite-combobox", () => {

expect(await combobox.getProperty("filteredItems")).toHaveLength(2);

const visibleItems = await page.findAll("calcite-combobox-item:not([hidden])");
const visibleItems = await page.findAll("calcite-combobox-item:not([hidden]):not([hidden-item])");

expect(visibleItems.map((item) => item.id)).toEqual(["text-label-match", "description-match"]);
});
Expand Down Expand Up @@ -919,6 +919,56 @@ describe("calcite-combobox", () => {
}
});

it("should show correct number of items when child hidden", async () => {
const page = await newE2EPage();
await page.setContent(html`
<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" hidden 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(html`
<calcite-combobox label="custom values" allow-custom-values placeholder="placeholder" max-items="6">
<calcite-combobox-item value="Trees" text-label="Trees" hidden>
<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
13 changes: 7 additions & 6 deletions packages/calcite-components/src/components/combobox/combobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ import type { Chip } from "../chip/chip";
import type { ComboboxItemGroup as HTMLCalciteComboboxItemGroupElement } from "../combobox-item-group/combobox-item-group";
import type { ComboboxItem as HTMLCalciteComboboxItemElement } from "../combobox-item/combobox-item";
import type { Label } from "../label/label";
import { isHidden } from "../../utils/component";
import T9nStrings from "./assets/t9n/messages.en.json";
import { ComboboxChildElement, GroupData, ItemData, SelectionDisplay } from "./interfaces";
import { ComboboxItemGroupSelector, ComboboxItemSelector, CSS, IDS } from "./resources";
Expand Down Expand Up @@ -145,20 +146,20 @@ export class Combobox

itemsAndGroups.forEach((item) => {
if (matchAll) {
item.hidden = false;
item.hiddenItem = false;
return;
}

const hidden = !find(item, filteredData);
item.hidden = hidden;
item.hiddenItem = hidden;
const [parent, grandparent] = item.ancestors;

if (find(parent, filteredData) || find(grandparent, filteredData)) {
item.hidden = false;
item.hiddenItem = false;
}

if (!hidden) {
item.ancestors.forEach((ancestor) => (ancestor.hidden = false));
item.ancestors.forEach((ancestor) => (ancestor.hiddenItem = false));
}
});

Expand Down Expand Up @@ -1126,7 +1127,7 @@ export class Combobox

private getMaxScrollerHeight(): number {
const allItemsAndGroups = [...this.groupItems, ...this.getItems(true)];
const items = allItemsAndGroups.filter((item) => !item.hidden);
const items = allItemsAndGroups.filter((item) => !isHidden(item));

const { maxItems } = this;

Expand Down Expand Up @@ -1226,7 +1227,7 @@ export class Combobox
}

private getFilteredItems(): HTMLCalciteComboboxItemElement["el"][] {
return this.filterText === "" ? this.items : this.items.filter((item) => !item.hidden);
return this.filterText === "" ? this.items : this.items.filter((item) => !isHidden(item));
}

private updateItems = debounce((): void => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ describe("calcite-input-time-zone", () => {
}

let matchedTimeZoneItems = await page.findAll(
"calcite-input-time-zone >>> calcite-combobox-item:not([hidden])",
"calcite-input-time-zone >>> calcite-combobox-item:not([hidden]):not([hidden-item])",
);
expect(matchedTimeZoneItems.length).toBeGreaterThan(1);

Expand All @@ -234,7 +234,9 @@ describe("calcite-input-time-zone", () => {
await page.waitForChanges();
await page.waitForTimeout(DEBOUNCE.filter);

matchedTimeZoneItems = await page.findAll("calcite-input-time-zone >>> calcite-combobox-item:not([hidden])");
matchedTimeZoneItems = await page.findAll(
"calcite-input-time-zone >>> calcite-combobox-item:not([hidden]):not([hidden-item])",
);

expect(matchedTimeZoneItems).toHaveLength(1);

Expand All @@ -243,7 +245,9 @@ describe("calcite-input-time-zone", () => {
await page.waitForChanges();
await page.waitForTimeout(DEBOUNCE.filter);

matchedTimeZoneItems = await page.findAll("calcite-input-time-zone >>> calcite-combobox-item:not([hidden])");
matchedTimeZoneItems = await page.findAll(
"calcite-input-time-zone >>> calcite-combobox-item:not([hidden]):not([hidden-item])",
);

expect(matchedTimeZoneItems).toHaveLength(1);

Expand All @@ -252,15 +256,19 @@ describe("calcite-input-time-zone", () => {
await page.waitForChanges();
await page.waitForTimeout(DEBOUNCE.filter);

matchedTimeZoneItems = await page.findAll("calcite-input-time-zone >>> calcite-combobox-item:not([hidden])");
matchedTimeZoneItems = await page.findAll(
"calcite-input-time-zone >>> calcite-combobox-item:not([hidden]):not([hidden-item])",
);

expect(matchedTimeZoneItems).toHaveLength(2);

await clearSearchTerm(searchTerms[1]);
await page.waitForChanges();
await page.waitForTimeout(DEBOUNCE.filter);

matchedTimeZoneItems = await page.findAll("calcite-input-time-zone >>> calcite-combobox-item:not([hidden])");
matchedTimeZoneItems = await page.findAll(
"calcite-input-time-zone >>> calcite-combobox-item:not([hidden]):not([hidden-item])",
);

expect(matchedTimeZoneItems.length).toBeGreaterThan(1);
});
Expand Down Expand Up @@ -545,7 +553,7 @@ describe("calcite-input-time-zone", () => {
await page.waitForTimeout(DEBOUNCE.filter);

const sharedOffsetTimeZoneItems = await page.findAll(
"calcite-input-time-zone >>> calcite-combobox-item:not([hidden])",
"calcite-input-time-zone >>> calcite-combobox-item:not([hidden]):not([hidden-item])",
);
expect(sharedOffsetTimeZoneItems).toHaveLength(2);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,3 +407,4 @@
}

@include base-component();
@include hidden-item();
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
import { IconNameOrString } from "../icon/interfaces";
import { useT9n } from "../../controllers/useT9n";
import type { Stepper } from "../stepper/stepper";
import { isHidden } from "../../utils/component";
import { CSS } from "./resources";
import T9nStrings from "./assets/t9n/messages.en.json";
import { styles } from "./stepper-item.scss";
Expand Down Expand Up @@ -92,6 +93,13 @@ 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
*
* @private
* */
@property({ reflect: true }) hiddenItem = false;

/**
* Specifies the layout of the `calcite-stepper-item` inherited from parent `calcite-stepper`, defaults to `horizontal`.
*
Expand Down Expand Up @@ -274,6 +282,7 @@ export class StepperItem extends LitElement implements InteractiveComponent, Loa
private handleItemClick(event: MouseEvent): void {
if (
this.disabled ||
isHidden(this.el) ||
(this.layout === "horizontal" &&
event
.composedPath()
Expand Down Expand Up @@ -303,9 +312,11 @@ 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:not([hidden]):not([hidden-item])",
),
).indexOf(this.el);
}

// #endregion
Expand Down
Loading
Loading