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

refactor(dom): Update getSlotted utility to handle querying for item(s) in default slot #3788

Merged
merged 18 commits into from
Jan 7, 2022
Merged

refactor(dom): Update getSlotted utility to handle querying for item(s) in default slot #3788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e304b8b
refactor(dom): Update getSlotted utility to handle querying for item(…
driskull Dec 28, 2021
a544156
cleanup
driskull Dec 28, 2021
4412bcb
fix tests
driskull Dec 28, 2021
01780f4
fix typing
driskull Dec 28, 2021
79b4cf8
Merge branch 'master' into dris0000/default-slot-query
driskull Dec 29, 2021
9cb29d1
add test
driskull Dec 29, 2021
d9a7b62
Merge branch 'master' into dris0000/default-slot-query
driskull Dec 29, 2021
ee92c3f
review fixes
driskull Dec 29, 2021
2b51a40
check assigned slot if browser
driskull Dec 29, 2021
f9f8cdf
revert
driskull Dec 30, 2021
9c14d47
Merge branch 'master' into dris0000/default-slot-query
driskull Dec 30, 2021
124d826
Merge branch 'master' into dris0000/default-slot-query
driskull Dec 30, 2021
1c547a0
test single slot
driskull Dec 30, 2021
5b8fcba
try again
driskull Dec 30, 2021
04828b7
Merge branch 'master' into dris0000/default-slot-query
driskull Jan 7, 2022
f16e601
fix test
driskull Jan 7, 2022
6149935
Merge branch 'master' into dris0000/default-slot-query
driskull Jan 7, 2022
8b9b31d
fix test?
driskull Jan 7, 2022
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
10 changes: 5 additions & 5 deletions src/components/calcite-dropdown/calcite-dropdown.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ describe("calcite-dropdown", () => {
describe("opens the dropdown with click, enter, or space", () => {
it("opens when dropdown-trigger is a button", async () => {
const page = await newE2EPage();
await page.setContent(html`
await page.setContent(html`
<calcite-dropdown>
<calcite-button slot="dropdown-trigger">Open dropdown</calcite-button>
<calcite-dropdown-group selection-mode="single">
Expand All @@ -768,7 +768,7 @@ describe("calcite-dropdown", () => {
</calcite-dropdown-item>
</calcite-dropdown-group>
</calcite-dropdown>
</calcite-dropdown>
</calcite-dropdown>
`);
const element = await page.find("calcite-dropdown");
const trigger = await element.find("calcite-button[slot='dropdown-trigger']");
Expand All @@ -794,9 +794,9 @@ describe("calcite-dropdown", () => {

it("opens when dropdown-trigger is an action", async () => {
const page = await newE2EPage();
await page.setContent(html`
await page.setContent(html`
<calcite-dropdown>
<calcite-action slot="dropdown-trigger">Open dropdown</calcite-button>
<calcite-action slot="dropdown-trigger">Open dropdown</calcite-action>
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 missed this in review :( cc @Elijbet

<calcite-dropdown-group selection-mode="single">
<calcite-dropdown-item id="item-1">
Dropdown Item Content
Expand All @@ -806,7 +806,7 @@ describe("calcite-dropdown", () => {
</calcite-dropdown-item>
</calcite-dropdown-group>
</calcite-dropdown>
</calcite-dropdown>
</calcite-dropdown>
`);
const element = await page.find("calcite-dropdown");
const trigger = await element.find("calcite-action[slot='dropdown-trigger'] >>> button");
Expand Down
18 changes: 17 additions & 1 deletion src/utils/dom.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,11 @@ describe("dom", () => {
}

connectedCallback(): void {
this.shadowRoot.innerHTML = `<slot name="${testSlotName}"></slot><slot name="${testSlotName2}"></slot>`;
this.shadowRoot.innerHTML = html`
<slot name="${testSlotName}"></slot>
<slot name="${testSlotName2}"></slot>
<slot></slot>
`;
}
}

Expand All @@ -119,11 +123,17 @@ describe("dom", () => {
</h2>
<h2 slot=${testSlotName}><span>😂</span></h2>
<h3 slot=${testSlotName2}><span>😂</span></h3>
<div id="default-slot-el"><p>🙂</p></div>
</slot-test>
`;

const assignedSlot = document.querySelector("slot-test").shadowRoot.querySelector(`slot:not([name])`);
Copy link
Member

Choose a reason for hiding this comment

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

Is this a workaround for Node.assignedSlot missing? If so, just wanted to check if it's working or not based on your comments to move to an e2e test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is a workaround.

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, the assignedNode won't be there. So we would have to run the getSlotted() method in the browser. Which means placing the whole utility in the browser so that all methods can be accessed.

(document.getElementById("default-slot-el") as any).assignedSlot = assignedSlot;
});

describe("single slotted", () => {
driskull marked this conversation as resolved.
Show resolved Hide resolved
it("returns elements with matching default slot", () => expect(getSlotted(getTestComponent())).toBeTruthy());

it("returns elements with matching slot name", () =>
expect(getSlotted(getTestComponent(), testSlotName)).toBeTruthy());

Expand All @@ -133,6 +143,9 @@ describe("dom", () => {
it("returns null when no results", () => expect(getSlotted(getTestComponent(), "non-existent-slot")).toBeNull());

describe("scoped selector", () => {
it("returns element with matching default slot", () =>
expect(getSlotted(getTestComponent(), { selector: "p" })).toBeTruthy());

it("returns element with matching nested selector", () =>
expect(getSlotted(getTestComponent(), testSlotName, { selector: "span" })).toBeTruthy());

Expand Down Expand Up @@ -182,6 +195,9 @@ describe("dom", () => {
});

describe("multiple slotted", () => {
it("returns element with default slot name", () =>
expect(getSlotted(getTestComponent(), { all: true })).toHaveLength(1));

it("returns elements with matching slot name", () =>
expect(getSlotted(getTestComponent(), testSlotName, { all: true })).toHaveLength(2));

Expand Down
32 changes: 25 additions & 7 deletions src/utils/dom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,24 +162,33 @@ interface GetSlottedOptions {
selector?: string;
}

const defaultSlotSelector = "> :not([slot])";

export function getSlotted<T extends Element = Element>(
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 should be typed like so:

export function getSlotted<T extends Element = Element>(element: Element, options: GetSlottedOptions & { all: true }): T[];
export function getSlotted<T extends Element = Element>(element: Element, options?: GetSlottedOptions): T[];
export function getSlotted<T extends Element = Element>(element: Element, slotName: string | string[], options: GetSlottedOptions & { all: true }): T[];
export function getSlotted<T extends Element = Element>(element: Element, slotName: string | string[], options?: GetSlottedOptions): T[];
export function getSlotted<T extends Element = Element>(
  element: Element,
  slotNameOrOptions: string | string[] | GetSlottedOptions,
  optionsOrUndefined?: GetSlottedOptions): (T | null) | T[] { /* ... */ }

Can you create a separate refactor issue this one and assign it to me?

element: Element,
slotName: string | string[],
slotName: string | string[] | (GetSlottedOptions & { all: true }),
options: GetSlottedOptions & { all: true }
): T[];
export function getSlotted<T extends Element = Element>(
element: Element,
slotName: string | string[],
slotName?: string | string[] | GetSlottedOptions,
options?: GetSlottedOptions
): T | null;
export function getSlotted<T extends Element = Element>(
element: Element,
slotName: string | string[],
slotName?: string | string[] | GetSlottedOptions,
options?: GetSlottedOptions
): (T | null) | T[] {
const slotSelector = Array.isArray(slotName)
? slotName.map((name) => `[slot="${name}"]`).join(",")
: `[slot="${slotName}"]`;
if (!Array.isArray(slotName) && typeof slotName !== "string") {
options = slotName;
slotName = null;
}

const slotSelector = slotName
? Array.isArray(slotName)
? slotName.map((name) => `[slot="${name}"]`).join(",")
: `[slot="${slotName}"]`
: defaultSlotSelector;

if (options?.all) {
return queryMultiple<T>(element, slotSelector, options);
Expand All @@ -196,6 +205,10 @@ function queryMultiple<T extends Element = Element>(
let matches = Array.from(element.querySelectorAll<T>(slotSelector));
matches = options && options.direct === false ? matches : matches.filter((el) => el.parentElement === element);

if (slotSelector === defaultSlotSelector) {
matches = matches.filter((match) => match?.assignedSlot);
Copy link
Member Author

Choose a reason for hiding this comment

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

@jcfranco It seems like this would work but we would need to move the spec tests to e2e for assignedSlot to work I think.

}

const selector = options?.selector;
return selector
? matches
Expand All @@ -211,10 +224,15 @@ function querySingle<T extends Element = Element>(
options?: GetSlottedOptions
): T | null {
let match = element.querySelector<T>(slotSelector);

if (slotSelector === defaultSlotSelector) {
match = match?.assignedSlot ? match : null;
}

match = options && options.direct === false ? match : match?.parentElement === element ? match : null;

const selector = options?.selector;
return selector ? match.querySelector<T>(selector) : match;
return selector ? match?.querySelector<T>(selector) : match;
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 default slot selector will also return children in a custom element without a <slot>, this should also make sure that assignedSlot from the default slot selector result is not null.

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 don't think the assignedSlot will work on the spec test. I think we would have to setup an e2e test?

}

export function filterDirectChildren<T extends Element>(el: Element, selector: string): T[] {
Expand Down