refactor: Update relevant components to handle slotting after init #3889

driskull · 2022-01-10T21:44:15Z

Related Issue: #3686

Summary

refactor: Update relevant components to use ConditionalSlotComponent

Also spawned: #3881

driskull

@jcfranco I think this needs your mutation observer fixes for conditionalSlot. Some getSlotted are not working correctly. I'll wait for that to get in.

driskull · 2022-01-10T22:35:51Z

src/utils/dom.ts

@@ -159,10 +159,11 @@ export async function focusElement(el: CalciteFocusableElement): Promise<void> {
 interface GetSlottedOptions {
  all?: boolean;
  direct?: boolean;
+  matches?: string;


@jcfranco what do you think? Should we have a way to do getSlotted(this.el, { matches: "calcite-link"});

I think selector should work here. Let me know if it doesn't match your needs (badumtss🥁) .

@jcfranco selector queries the element for children, matches tests the element.

the matches is more of a filter.

Gotcha. Looks like you found a hole in this util. 🍩 I think queries using selector should include the slotted element. WDYT? Fortunately, there are only 2 selector usages so far:

https://github.com/Esri/calcite-components/blob/2ec984876f33939d06c7ad0b47c9040f1a7d0a9d/src/components/calcite-tree-item/calcite-tree-item.tsx#L414

https://github.com/Esri/calcite-components/blob/5e0f9f259d160f7826dbf1be56b05ad52b0d0add/src/components/calcite-alert/calcite-alert.tsx#L241

I'll create an issue for the above and can tackle this separately.

Yeah we can either update how selector works or matches.

driskull · 2022-01-10T22:36:07Z

src/utils/dom.ts

  selector?: string;
 }

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


I'm not sure why I had the > but this might be something we need??

Ok, so can't do direct selector because of the scope. Instead, have to filter child nodes first.

I tried using :scope > but that wasn't working either.
https://developer.mozilla.org/en-US/docs/Web/CSS/:scope

Interesting. Do the current spec tests cover this? Wonder why it didn't pick this up.

Do the current spec tests cover this? Wonder why it didn't pick this up.

We weren't using a selector like this for all default slot queries.

driskull · 2022-01-10T22:36:31Z

src/components/calcite-action-menu/calcite-action-menu.tsx

-      : (Array.from(
-          el.querySelectorAll("calcite-action:not([slot=trigger])")
-        ) as HTMLCalciteActionElement[]);
+    const actionElements = getSlotted(el, { all: true, matches: "calcite-action" }) as any;


Need to test this cleanup...

Need to test this cleanup...

Test successful?

Also, can you type provide the type in the helper?

const actionElements = getSlotted<HTMLCalciteAction>(/* ... */);

I think this issue needs to happen first.... #3881

Test successful?

Yes this seems to be working.

Also, can you type provide the type in the helper?

The typings need some updates so that it correctly reports an array of items. Currently, it thinks its a single element.

src/components/calcite-alert/calcite-alert.tsx

src/components/calcite-inline-editable/calcite-inline-editable.tsx

src/components/calcite-input/calcite-input.tsx

src/components/calcite-notice/calcite-notice.tsx

src/components/calcite-radio-button-group/calcite-radio-button-group.tsx

jcfranco · 2022-01-13T19:42:37Z

#3871 just landed, so you the util should work more reliably and you now have access to the test helper (for named slots) if needed.

# Conflicts: # src/components/calcite-inline-editable/calcite-inline-editable.tsx

jcfranco · 2022-01-19T01:45:04Z

src/components/calcite-action-menu/calcite-action-menu.tsx

-      : (Array.from(
-          el.querySelectorAll("calcite-action:not([slot=trigger])")
-        ) as HTMLCalciteActionElement[]);
+    const actionElements = getSlotted(el, { all: true, matches: "calcite-action" }) as any;


Need to test this cleanup...

Test successful?

Also, can you type provide the type in the helper?

const actionElements = getSlotted<HTMLCalciteAction>(/* ... */);

src/components/calcite-combobox-item/calcite-combobox-item.tsx

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

src/components/calcite-inline-editable/calcite-inline-editable.tsx

src/components/calcite-input/calcite-input.tsx

src/components/calcite-combobox-item/calcite-combobox-item.tsx

src/components/calcite-card/calcite-card.tsx

src/components/calcite-radio-button/calcite-radio-button.tsx

jcfranco · 2022-01-19T03:05:37Z

src/utils/dom.ts

@@ -159,10 +159,11 @@ export async function focusElement(el: CalciteFocusableElement): Promise<void> {
 interface GetSlottedOptions {
  all?: boolean;
  direct?: boolean;
+  matches?: string;


Gotcha. Looks like you found a hole in this util. 🍩 I think queries using selector should include the slotted element. WDYT? Fortunately, there are only 2 selector usages so far:

https://github.com/Esri/calcite-components/blob/2ec984876f33939d06c7ad0b47c9040f1a7d0a9d/src/components/calcite-tree-item/calcite-tree-item.tsx#L414

https://github.com/Esri/calcite-components/blob/5e0f9f259d160f7826dbf1be56b05ad52b0d0add/src/components/calcite-alert/calcite-alert.tsx#L241

I'll create an issue for the above and can tackle this separately.

jcfranco · 2022-01-19T03:15:22Z

src/utils/dom.ts

  selector?: string;
 }

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


Interesting. Do the current spec tests cover this? Wonder why it didn't pick this up.

eriklharper · 2022-01-21T22:04:50Z

/merge master

# Conflicts: # src/components/calcite-input/calcite-input.tsx

driskull · 2022-01-21T23:42:39Z

@jcfranco anything else I need to do to get this one merged?

jcfranco

Just had one suggestion, but this LGTM! 🎉

Anything else that came up will be tackled separately.

jcfranco · 2022-01-21T23:55:33Z

src/components/calcite-radio-button-group/calcite-radio-button-group.tsx

@@ -81,6 +90,11 @@ export class CalciteRadioButtonGroup {

  connectedCallback(): void {
    this.passPropsToRadioButtons();
+    this.mutationObserver?.observe(this.el, { childList: true, subtree: true });


Do you need to watch for changes to radio-button children? I don't think it supports this, right?

@jcfranco I don't think so. The props should only go down the dom right?

The props should only go down the dom right?

If I understand correctly, only one level, yes.

…3889) * refactor: Update relevant components to use ConditionalSlotComponent #3686 * cleanup * cleanup * cleanup * fix links that aren't direct slots * remove assigned slot check, only get direct children * fix test * cleanup * cleanup * partial review fixes * revert changes to combobox and card

* fix(label, input, input-message)!: deprecate and no longer support status prop for calcite-label * alter tests for status being passed in calcite label * update the readme for labels using status and add status prop to input/input-message * add deprecated comment for status prop * refactor(panel, shell, tree-item): Add keys to conditionally rendered slots. (#3962) * refactor(panel, shell, tree-item): Add keys to conditionally rendered slots. * add const for key. * fix code * review cleanup * feat(panel): Add method to scroll content. #3924 (#3960) * ci(screener): updating to Chrome 97 (#3940) * 1.0.0-next.373 * docs: update component READMEs (#3972) Co-authored-by: jcfranco <jcfranco@users.noreply.github.com> * ci(screener): enlarging screen resolution (#3977) * refactor(input): fix all types that are any (#3981) * refactor: Update relevant components to handle slotting after init (#3889) * refactor: Update relevant components to use ConditionalSlotComponent #3686 * cleanup * cleanup * cleanup * fix links that aren't direct slots * remove assigned slot check, only get direct children * fix test * cleanup * cleanup * partial review fixes * revert changes to combobox and card * refactor(slider): remove drag event listeners on component disconnected (#3969) * refactor(slider): remove eventListeners on DOM disconnected * feedback changes * add private method for duplicate code * rename method * build: update browserslist db (#3985) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * test(input): when both 'ArrowUp' and 'ArrowDown' are pressed at the same time most recently pressed key takes over (#3989) Co-authored-by: Eliza Khachatryan <eli97736@esri.com> * alter verbiage of deprecated prop * ci(screener): restoring pre-Sauce integration settings (#3994) * docs(template): provide context for the repro samples (#3970) * docs(templates): provide content for the repro samples * use cc latest version Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * chore: pin node 16 and npm 8 for volta (#3964) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * docs(readme): remove sketch info until it is up to date (#3996) * fix(date-picker): update utils locale to get the lang code if regional code is not found (#3968) * fix(date-picker): update utils locale to get the lang code if regional code is not found * alter util changes based from pr comments * 1.0.0-next.374 * ci: allow primary and secondary contacts to merge on all prs (#3999) * ci: allow primary and secondary contacts to merge on all prs * cleanup * use admin token * fix(date-picker, input-date-picker): update pt-BR and pt-PT localization files (#3980) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * feat(tree): multi-selection no longer requires holding shift key (#3733) * fix(tree): enable multi select with pointer * save selected items at main level * fix single selection mode * deselect siblings when mode is single * fix deselecting tree item when mode is multi * fix deselecting with shift keydown * refactor e2e test * remove unused imports in e2e * feedback changes * emit calciteTreeSelect event without shift key * refactor code * add e2e test and feedback changes Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * 1.0.0-next.375 * test(dropdown): correct logical errors in the test (#3998) * test(dropdown): correct logical errors in the test * assert both events after each toggle action and use page.waitForEvent before doing assertions Co-authored-by: Eliza Khachatryan <eli97736@esri.com> * deprecate and restore the use of status prop * restore reading status prop from label Co-authored-by: Matt Driscoll <mdriscoll@esri.com> Co-authored-by: Erik Harper <eharperdicianno@esri.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Ben Elan <benelan@users.noreply.github.com> Co-authored-by: jcfranco <jcfranco@users.noreply.github.com> Co-authored-by: Anveshreddy mekala <anv11827@esri.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Eliza Khachatryan <elijbet@gmail.com> Co-authored-by: Eliza Khachatryan <eli97736@esri.com> Co-authored-by: JC Franco <jfranco@esri.com>

refactor: Update relevant components to use ConditionalSlotComponent #…

c7390c6

…3686

driskull self-assigned this Jan 10, 2022

github-actions bot added this to the Sprint 01/03 - 01/14 milestone Jan 10, 2022

github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Jan 10, 2022

driskull added 2 commits January 10, 2022 14:34

cleanup

25c0124

cleanup

880461d

driskull commented Jan 10, 2022

View reviewed changes

driskull added 2 commits January 11, 2022 13:46

Merge branch 'master' into dris0000/conditional-slot-round-2

8e2fcb6

Merge branch 'master' into dris0000/conditional-slot-round-2

5073377

driskull added 10 commits January 13, 2022 11:48

Merge branch 'master' into dris0000/conditional-slot-round-2

5e0f9f2

# Conflicts: # src/components/calcite-inline-editable/calcite-inline-editable.tsx

Merge branch 'master' into dris0000/conditional-slot-round-2

52aef5e

cleanup

f7e8c64

fix links that aren't direct slots

501207a

remove assigned slot check, only get direct children

1daf449

Merge branch 'master' into dris0000/conditional-slot-round-2

c082918

Merge branch 'master' into dris0000/conditional-slot-round-2

fc31aed

fix test

15099d1

cleanup

dfa0771

cleanup

e0846b1

driskull requested a review from jcfranco January 18, 2022 19:29

driskull marked this pull request as ready for review January 19, 2022 01:18

driskull requested a review from a team as a code owner January 19, 2022 01:18

jcfranco reviewed Jan 19, 2022

View reviewed changes

driskull added 3 commits January 19, 2022 09:40

Merge branch 'master' into dris0000/conditional-slot-round-2

f13629b

partial review fixes

8863b86

revert changes to combobox and card

6919590

driskull changed the title ~~refactor: Update relevant components to use ConditionalSlotComponent~~ refactor: Update relevant components to handle slotting after init Jan 19, 2022

driskull added 2 commits January 19, 2022 15:16

Merge branch 'master' into dris0000/conditional-slot-round-2

d214ff5

Merge branch 'master' into dris0000/conditional-slot-round-2

3f1badc

driskull added 2 commits January 19, 2022 17:36

Merge branch 'master' into dris0000/conditional-slot-round-2

a514c62

Merge branch 'master' into dris0000/conditional-slot-round-2

2af4579

driskull requested a review from eriklharper January 20, 2022 19:42

jcfranco modified the milestones: Sprint 01/03 - 01/14, Sprint 01/17 - 01/28 Jan 20, 2022

Merge branch 'master' into dris0000/conditional-slot-round-2

52a369b

driskull mentioned this pull request Jan 20, 2022

calcite-inline-editable throws error if no calcite-input is slotted #3780

Closed

2 tasks

Merge branch 'master' into dris0000/conditional-slot-round-2

e91cf08

driskull requested a review from jcfranco January 21, 2022 18:08

Merge branch 'master' into dris0000/conditional-slot-round-2

666bb2b

# Conflicts: # src/components/calcite-input/calcite-input.tsx

jcfranco approved these changes Jan 21, 2022

View reviewed changes

driskull merged commit b00bdf7 into master Jan 22, 2022

driskull deleted the dris0000/conditional-slot-round-2 branch January 22, 2022 00:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Update relevant components to handle slotting after init #3889

refactor: Update relevant components to handle slotting after init #3889

driskull commented Jan 10, 2022 •

edited

Loading

driskull left a comment

driskull Jan 10, 2022

jcfranco Jan 13, 2022

driskull Jan 13, 2022

driskull Jan 13, 2022

jcfranco Jan 19, 2022

driskull Jan 19, 2022

driskull Jan 10, 2022

driskull Jan 14, 2022

jcfranco Jan 19, 2022

driskull Jan 19, 2022

driskull Jan 10, 2022

jcfranco Jan 19, 2022

driskull Jan 19, 2022

driskull Jan 19, 2022

driskull Jan 21, 2022

jcfranco commented Jan 13, 2022

jcfranco Jan 19, 2022

jcfranco Jan 19, 2022

jcfranco Jan 19, 2022

eriklharper commented Jan 21, 2022

driskull commented Jan 21, 2022

jcfranco left a comment

jcfranco Jan 21, 2022

driskull Jan 22, 2022

jcfranco Jan 22, 2022

refactor: Update relevant components to handle slotting after init #3889

refactor: Update relevant components to handle slotting after init #3889

Conversation

driskull commented Jan 10, 2022 • edited Loading

Summary

driskull 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

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

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

Choose a reason for hiding this comment

jcfranco commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriklharper commented Jan 21, 2022

driskull commented Jan 21, 2022

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

driskull commented Jan 10, 2022 •

edited

Loading