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

driskull · 2021-12-28T00:27:54Z

Related Issue: None

Summary

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

Update dom utility getSlotted to query for an element in the default slot.
make slotName param optional

This will allow us to replace this.hasDefaultSlot = this.el.querySelector(":not([slot])") !== null; taken from combobox-item.

…s) in the default slot.

jcfranco

This is looking great! This will be a nice update to getSlotted. 🎉

jcfranco · 2021-12-29T16:38:37Z

src/utils/dom.ts

@@ -185,7 +187,7 @@ function querySingle<T extends Element = Element>(
  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;


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.

I don't think the assignedSlot will work on the spec test. I think we would have to setup an e2e test?

src/utils/dom.spec.ts

jcfranco · 2021-12-29T16:41:36Z

src/utils/dom.ts

  options?: GetSlottedOptions
 ): T | null;
 export function getSlotted<T extends Element = Element>(
  element: Element,
-  slotName: string,
+  slotName?: string,


Can you update the signature to support both

getSlotted(el, slotName, options); getSlotted(el, options);

?

Yeah, or we can just update the slotName to be apart of options? What do you think?

Do you have an example of making the second param be identified as the third?

src/utils/dom.spec.ts

# Conflicts: # src/utils/dom.spec.ts # src/utils/dom.ts

driskull · 2021-12-29T23:27:20Z

@jcfranco can you review again?

driskull · 2021-12-31T00:10:36Z

src/utils/dom.ts

@@ -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);


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

github-actions · 2022-01-07T01:54:31Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

driskull · 2022-01-07T23:07:01Z

src/components/calcite-dropdown/calcite-dropdown.e2e.ts

        <calcite-dropdown>
-          <calcite-action slot="dropdown-trigger">Open dropdown</calcite-button>
+          <calcite-action slot="dropdown-trigger">Open dropdown</calcite-action>


I missed this in review :( cc @Elijbet

jcfranco

Code changes LGTM!

For the remaining items I brought up (1. getSlotted signature, 2. assignedSlot check), can you create follow up issues?

jcfranco · 2022-01-07T23:17:46Z

src/utils/dom.spec.ts

      </slot-test>
    `;
+
+      const assignedSlot = document.querySelector("slot-test").shadowRoot.querySelector(`slot:not([name])`);


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.

Yes, this is a workaround.

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.

jcfranco · 2022-01-07T23:23:23Z

src/utils/dom.ts

@@ -162,24 +162,33 @@ interface GetSlottedOptions {
  selector?: string;
 }

+const defaultSlotSelector = "> :not([slot])";
+
 export function getSlotted<T extends Element = Element>(


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?

refactor(dom): Update getSlotted utility to handle querying for item(…

e304b8b

…s) in the default slot.

github-actions bot added this to the Sprint 12/20 - 12/31 milestone Dec 28, 2021

github-actions bot assigned driskull Dec 28, 2021

github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Dec 28, 2021

driskull added 2 commits December 27, 2021 16:38

cleanup

a544156

fix tests

4412bcb

driskull marked this pull request as ready for review December 28, 2021 18:40

driskull requested a review from a team as a code owner December 28, 2021 18:40

driskull requested a review from jcfranco December 28, 2021 18:40

fix typing

01780f4

jcfranco requested changes Dec 29, 2021

View reviewed changes

driskull added 5 commits December 29, 2021 12:08

Merge branch 'master' into dris0000/default-slot-query

79b4cf8

# Conflicts: # src/utils/dom.spec.ts # src/utils/dom.ts

add test

9cb29d1

Merge branch 'master' into dris0000/default-slot-query

d9a7b62

review fixes

ee92c3f

check assigned slot if browser

2b51a40

driskull requested a review from jcfranco December 29, 2021 23:25

driskull added 5 commits December 29, 2021 16:26

revert

f9f8cdf

Merge branch 'master' into dris0000/default-slot-query

9c14d47

Merge branch 'master' into dris0000/default-slot-query

124d826

test single slot

1c547a0

try again

5b8fcba

driskull commented Dec 31, 2021

View reviewed changes

Merge branch 'master' into dris0000/default-slot-query

04828b7

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Jan 7, 2022

driskull added 3 commits January 7, 2022 12:54

fix test

f16e601

Merge branch 'master' into dris0000/default-slot-query

6149935

fix test?

8b9b31d

driskull commented Jan 7, 2022

View reviewed changes

jcfranco approved these changes Jan 7, 2022

View reviewed changes

This was referenced Jan 7, 2022

Refactor getSlotted dom utility method #3881

Closed

Refactor getSlotted dom tests to run in browser #3882

Closed

driskull merged commit 26c926b into master Jan 7, 2022

driskull deleted the dris0000/default-slot-query branch January 7, 2022 23:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

driskull commented Dec 28, 2021 •

edited

Loading

jcfranco left a comment

jcfranco Dec 29, 2021

driskull Dec 29, 2021

jcfranco Dec 29, 2021

driskull Dec 29, 2021

driskull commented Dec 29, 2021

driskull Dec 31, 2021

github-actions bot commented Jan 7, 2022

driskull Jan 7, 2022

jcfranco left a comment

jcfranco Jan 7, 2022

driskull Jan 7, 2022

driskull Jan 7, 2022

jcfranco Jan 7, 2022

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

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

Conversation

driskull commented Dec 28, 2021 • edited Loading

Summary

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driskull commented Dec 29, 2021

Choose a reason for hiding this comment

github-actions bot commented Jan 7, 2022

Choose a reason for hiding this comment

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driskull commented Dec 28, 2021 •

edited

Loading