Toast: Update Toaster to use internally consistent scoping

[daneah]

This change: (check at least one)

• Adds a new feature
• Fixes a bug
• Improves maintainability
• Improves documentation
• Is a release activity

Is this a breaking change? (check one)

• Yes
• No

Is the: (complete all)

• Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
• Test suite(s) passing?
• Code coverage maximal?
• Changeset added?
• Component status page up to date?

What does this change address?

Fixes #474

How does this change work?

Updates PharosToaster to use ScopedRegistryMixin and replace (most of) the DOM manipulation it was doing with declarative HTML rendering.

Additional context

This is a breaking change because it changes the detail value for the pharos-toast-close event. Because this event was externally visible, this is technically an API change. I don't, however, believe any internal consumers were listening for this event. Because we're gearing up for v13 and this isn't otherwise too pressing, let's just wait and include it there.

[changeset-bot]

🦋 Changeset detected

Latest commit: d45f00f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@ithaka/pharos	Major
@ithaka/pharos-site	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
packages/pharos/lib/index.js	48.96 KB (+2.32% 🔺)

[daneah]

Open to toning this down or suggestions on where to move these—we don't have an established pattern. I could see there being a good case for something like src/components/<comp>/pharos-<comp>.types.ts.

[eslawski]

Personally like to keep types close to where they are used. We could consider moving them out into their own module when we need to use them elsewhere or wish to expose them to the consumer?

[daneah]

These were missing before.

[daneah]

I think this was errantly placed here; I moved it to the component's JSDoc block below.

[daneah]

@Niznikr curious if you might understand more about why this is and if I've missed some trick here—the following did not work, failing to render e.g. a pharos-link present in toast.content:

    return html`<pharos-toast
      id="${toast.id}"
      status="${toast.status}"
      ?indefinite="${toast.indefinite}"
    >
      ${unsafeHTML(toast.content)}
    </pharos-toast>`;

[Niznikr]

Not certain tbh. Believe I ran into this the first time around. The Lit Discord might be helpful.

[daneah]

Posed the question here.

[daneah]

Interestingly this doesn't happen in the Lit playground.

[chrisjbrown]

is this because pharos-link isn't being registered anywhere in the story?

if I add PharosLink as a "local" definition in the PharosToaster. it does render correctly

export class PharosToaster extends ScopedRegistryMixin(PharosElement) {
  static elementDefinitions = {
    'pharos-toast': PharosToast,
    'pharos-link': PharosLink,
  };

** just did this to test to see if it would work, not suggesting this

[daneah]

is this because pharos-link isn't being registered anywhere in the story?

The components are all registered and available to all stories by nature of this module and its import.

if I add PharosLink as a "local" definition in the PharosToaster. it does render correctly

To be clear, it already does render correctly currently. The question is whether there's a way to avoid the DOM manipulation here while maintaining the functionality.

The crux being that the supplied content comes from the consumer, and the toast cannot predict what that content will be, and therefore does not want to be in the business of e.g. registering all possible components that could be in that content.

[chrisjbrown]

in my testing i added this

export class PharosToaster extends ScopedRegistryMixin(PharosElement) {
  static elementDefinitions = {
    'pharos-toast': PharosToast,
    'pharos-link': PharosLink,
  };

as well as changed the rendering back to

return html`<pharos-toast
      id="${toast.id}"
      status="${toast.status}"
      ?indefinite="${toast.indefinite}"
    >
      ${unsafeHTML(toast.content)}
    </pharos-toast>`;

and i'm wondering why does this render correctly and not render correctly if we don't set pharos-link in elementDefinitions if this module is doing that registering anyway

[daneah]

I have been assuming this is due to the supplied content being put into the Toast's light DOM, but now that you prompt me to think more about it the Toast is inside the Toaster's shadow DOM so I am not sure exactly what I should be expecting. The scope seems like it would indeed be the Toaster's registry.

[daneah]

(Updated in the Discord discussion as well) @chrisjbrown and I tracked this down to the scoped registry behavior, which we now understand does not have inheritance—you either get the global registry without isolation, or you get your total isolation with only explicit components allowed. The DOM manipulation style is what allows us to straddle the two.