refactor: use shared slot presence mixin for conditional element styling #824

Westbrook · 2020-08-12T14:27:38Z

Description

Catch up to main and the keyboard helper work that is there.
Move to using @spectrum-web-components/base as the only entry to lit-element and lit-html.
update to latest lit-html/lit-element
use new lit-element to power the below...

Querying light DOM children to resolve template content is not reactive:

private get hasFooter(): boolean {	
        return !!this.querySelector('[slot="footer"]');	
}

Meaning that changing the presence of this content dynamically will not inherently trigger a new render.

Relying on a MutationObserver instead will allow the value of hasFooter or the like to be dynamic to content changes and ensure a more correct delivery of the elements template across its lifecycle.

Refactors ObserveSlotText to better leverage queryAssignedNodes. I see the possibility of merging the two observer mixins in some way in the future, but for now the use of Symbol() based properties allows them to neatly avoid stepping on each other without further refactoring.

Related Issue

fixes #793

Motivation and Context

Templates should be correct. That's the tweet...oh, wait.

How Has This Been Tested?

existing tests
new tests

Types of changes

feature factor

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

andrewhatran · 2020-08-26T22:41:10Z

packages/shared/README.md

-### ObserverSlotText
+### ObserveSlotPresence
+
+When working with styles that are driven by the conditional presence of `<slot>`s in a component's shadow DOM, you will need to track whether light DOM that is meant for that slot. Use the `ObserveSlotPresence` mixin to target specific light DOM to observe the presence of and trigger `this.requestUpdate()` calls when content fulfilling that selector comes in and out of availability.


Did you mean:

you will need to track whether light DOM that is meant for that slot exists?

andrewhatran · 2020-08-26T22:49:17Z

packages/shared/src/observe-slot-text.ts

 const slotElementObserver = Symbol('slotElementObserver');
+const assignedNodesList = Symbol('assinedNodes');


Suggested change

const assignedNodesList = Symbol('assinedNodes');

const assignedNodesList = Symbol('assignedNodes');

andrewhatran · 2020-08-26T22:53:52Z

packages/base/README.md

@@ -34,6 +34,8 @@ export MyElement extends SpectrumMixin(HTMLElement) {}

 With powerful CSS selectors like `:host(:dir(rtl))` and `:host-content([dir=rtl])` not yet enjoying cross-browser support, reliably delivering content in both "left to right" and "right to left" while relying on Shadow DOM means certain trade offs need to be made. While every custom element build from `SpectrumMixin` could have its `dir` attribute applied to manage this, doing so is not only a pain for apply, it's a pain to maintain over time. To support the flexibility to devlier content in both of these directions, elements built from `SpectrumMixin` will observe the `dir` attribute of either the `document` or a containing `sp-theme`. This will allow your sites and applications to manage content direction in a single place while also opening the possibility of having multiple content directions on the same page by scoping those content sections with `sp-theme` elements.

+When placing a `dir` attribute on an element built from `SpectrumMixin` before attaching it to the DOM, this will prevent it from resolving the text direction value to a parent `sp-theme` or `document` element. Elements applied to the page in this way will also NOT participate in observing any such elements, so their `dir` values will remain as initialized regardless of changes in other parts of your documents. If you choose to leverage this, be aware that any child (in both light DOM and shadow DOM) of this element will need to have a `dir` attribute applied as well if you do not want it resolving to a parent `sp-theme` or `document` element itself. In this way, it is likely that you would benefit from leveraging an `sp-theme` element to create scope in your document for managing this custom content direction section of your page.


Suggested change

When placing a `dir` attribute on an element built from `SpectrumMixin` before attaching it to the DOM, this will prevent it from resolving the text direction value to a parent `sp-theme` or `document` element. Elements applied to the page in this way will also NOT participate in observing any such elements, so their `dir` values will remain as initialized regardless of changes in other parts of your documents. If you choose to leverage this, be aware that any child (in both light DOM and shadow DOM) of this element will need to have a `dir` attribute applied as well if you do not want it resolving to a parent `sp-theme` or `document` element itself. In this way, it is likely that you would benefit from leveraging an `sp-theme` element to create scope in your document for managing this custom content direction section of your page.

Placing a `dir` attribute on an element built from `SpectrumMixin` before attaching it to the DOM will prevent it from resolving the text direction value to a parent `sp-theme` or `document` element. Elements applied to the page in this way will also NOT participate in observing any such elements, so their `dir` values will remain as initialized regardless of changes in other parts of your documents. If you choose to leverage this, be aware that any child (in both light DOM and shadow DOM) of this element will need to have a `dir` attribute applied as well if you do not want it resolving to a parent `sp-theme` or `document` element itself. In this way, it is likely that you would benefit from leveraging an `sp-theme` element to create scope in your document for managing this custom content direction section of your page.

adixon-adobe · 2020-08-28T15:49:23Z

packages/split-button/README.md

@@ -1,11 +1,11 @@
 ## Description

-`sp-splitbutton` delivers a
+An `sp-split-button` surfaces an immediately envokable action via it's main button, as well as a list of alternative actions in its toggleable menu overlay. By default, any actions envoked from the overlay will replace the main action button. When leveraging `[type="more"]` the action will be envoked, but the main button will remain the action initally persribed by the implementor.


I'd suggest changing envoke to invoke since it's more common (and I know my spell check is really wanting to change it as I'm trying to give this feedback). I did learn today that envoke is a valid alternate spelling though 🌈

adixon-adobe

Looks good to me.

Westbrook force-pushed the westbrook/has branch 3 times, most recently from 76ceb7d to df31b8e Compare August 20, 2020 18:15

Westbrook changed the base branch from main to spectrum-css-3 August 20, 2020 23:28

Westbrook force-pushed the spectrum-css-3 branch from a837c71 to ad7a4a7 Compare August 20, 2020 23:40

Westbrook force-pushed the westbrook/has branch 4 times, most recently from 4063d8f to 9849083 Compare August 21, 2020 01:38

Westbrook changed the title ~~[blocked] refactor: use shared slot presence mixin for conditional element styling~~ refactor: use shared slot presence mixin for conditional element styling Aug 21, 2020

Westbrook requested a review from andrewhatran August 21, 2020 12:31

Westbrook mentioned this pull request Aug 21, 2020

Merge Target for Packages using the latest Spectrum CSS v3.0.0 dependency #790

Merged

Westbrook force-pushed the westbrook/has branch 2 times, most recently from 06465be to 86c6884 Compare August 21, 2020 19:17

Westbrook force-pushed the spectrum-css-3 branch from ad7a4a7 to c6f5efd Compare August 25, 2020 11:46

Westbrook force-pushed the westbrook/has branch from 86c6884 to 6ae6def Compare August 25, 2020 12:03

andrewhatran reviewed Aug 26, 2020

View reviewed changes

Westbrook force-pushed the spectrum-css-3 branch from c6f5efd to cf19168 Compare August 27, 2020 12:10

Westbrook mentioned this pull request Aug 27, 2020

Specified version of lit-element is not accurate #845

Closed

Westbrook force-pushed the westbrook/has branch from 6ae6def to 5e42cbe Compare August 27, 2020 12:19

Westbrook force-pushed the spectrum-css-3 branch from cf19168 to 26c50f6 Compare August 27, 2020 23:18

Westbrook and others added 7 commits August 28, 2020 00:05

feat(split-button): add split-button pattern

4f6566d

ci: update golden images cache

47c7fee

ci: update golden images cache

5e114b7

refactor: use shared slot presence mixin for conditional element styling

6e9d834

refactor: centralize on @spectrum-web-components/base for lit-* exports

7048222

refactor(base): export "ifDefined" from "lit-html"

4a009e6

docs: clarify language in README

309d17c

Westbrook force-pushed the westbrook/has branch from 5e42cbe to 309d17c Compare August 28, 2020 04:09

adixon-adobe reviewed Aug 28, 2020

View reviewed changes

adixon-adobe approved these changes Aug 28, 2020

View reviewed changes

Westbrook merged commit 189948b into spectrum-css-3 Aug 28, 2020

Westbrook deleted the westbrook/has branch August 28, 2020 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use shared slot presence mixin for conditional element styling #824

refactor: use shared slot presence mixin for conditional element styling #824

Westbrook commented Aug 12, 2020 •

edited

Loading

andrewhatran Aug 26, 2020

andrewhatran Aug 26, 2020

andrewhatran Aug 26, 2020

adixon-adobe Aug 28, 2020

adixon-adobe left a comment

		const slotElementObserver = Symbol('slotElementObserver');
		const assignedNodesList = Symbol('assinedNodes');

	const assignedNodesList = Symbol('assinedNodes');
	const assignedNodesList = Symbol('assignedNodes');

		@@ -34,6 +34,8 @@ export MyElement extends SpectrumMixin(HTMLElement) {}

		With powerful CSS selectors like `:host(:dir(rtl))` and `:host-content([dir=rtl])` not yet enjoying cross-browser support, reliably delivering content in both "left to right" and "right to left" while relying on Shadow DOM means certain trade offs need to be made. While every custom element build from `SpectrumMixin` could have its `dir` attribute applied to manage this, doing so is not only a pain for apply, it's a pain to maintain over time. To support the flexibility to devlier content in both of these directions, elements built from `SpectrumMixin` will observe the `dir` attribute of either the `document` or a containing `sp-theme`. This will allow your sites and applications to manage content direction in a single place while also opening the possibility of having multiple content directions on the same page by scoping those content sections with `sp-theme` elements.

		When placing a `dir` attribute on an element built from `SpectrumMixin` before attaching it to the DOM, this will prevent it from resolving the text direction value to a parent `sp-theme` or `document` element. Elements applied to the page in this way will also NOT participate in observing any such elements, so their `dir` values will remain as initialized regardless of changes in other parts of your documents. If you choose to leverage this, be aware that any child (in both light DOM and shadow DOM) of this element will need to have a `dir` attribute applied as well if you do not want it resolving to a parent `sp-theme` or `document` element itself. In this way, it is likely that you would benefit from leveraging an `sp-theme` element to create scope in your document for managing this custom content direction section of your page.

refactor: use shared slot presence mixin for conditional element styling #824

refactor: use shared slot presence mixin for conditional element styling #824

Conversation

Westbrook commented Aug 12, 2020 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

andrewhatran Aug 26, 2020

Choose a reason for hiding this comment

andrewhatran Aug 26, 2020

Choose a reason for hiding this comment

andrewhatran Aug 26, 2020

Choose a reason for hiding this comment

adixon-adobe Aug 28, 2020

Choose a reason for hiding this comment

adixon-adobe left a comment

Choose a reason for hiding this comment

Westbrook commented Aug 12, 2020 •

edited

Loading