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.

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

resolve multiple <slot> elements rendering on the home page #454

Merged

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Dec 24, 2020

Related Issue

resolves #433

Summary of Changes

Did some experimentation and comparisons between HTMLElement and LitElement (you can compare in the home page index.html right now).

From my research I learned / observed a couple interesting insights into Shadow DOM / <slot>

  1. <slot>s should be named

  2. They only seem to support a shallow level of nesting, even when using name, native HTMLElement and yarn develop (notice the third <slot> doesn't render)

    <script>
      class SlotTestNative extends HTMLElement {
        constructor() {
          super();
    
          // create a Shadow DOM
          this.root = this.attachShadow({ mode: 'closed' });
          this.root.innerHTML = `
            <h3>Content from the custom element. (inside HTMLElement)</h3>
              <h3>
                <slot name="content"></slot>
              <h3>
             <div>
               <span>nested - will the slot show?<span>
               <h3>
                 <slot name="content"></slot>
              <h3>
            </div>
          `;
        }
      }
    
      customElements.define('slot-test-native', SlotTestNative);
    </script>
    <slot-test-native>
      <h3 slot="content">Content for the slot. (outside HTMLElement)</h3>
    </slot-test-native>

Screen Shot 2020-12-24 at 11 39 21 AM

Screen Shot 2020-12-24 at 11 43 45 AM

So just had to do a little refactoring to our website HTML to better honor these rules.

  1. Removed extra <app-row> component to reduce extraneous nesting. _why use more JS when few JS do trick
  2. Adjust styles to reflect new file structure (could probably clean this up some more)
  3. Tested all major browsers (Edge, Chrome, FF, Safari) and all behaviors are consistent

TODO

  1. Update docs to discuss Shadow DOM / <slot> best practices, probably best to after merge of Docs/issue 430 update technical documentation to reflect current project status #452
  2. Remove <slot-test-native> example code after team review
  3. Should track an issue to investigate removing puppeteers pre generated CSS. It looks like this includes ShadowDOM CSS which while great, we really don't want to have in the global scope anyway, which is the whole point. The trade-off I observed is that when using CSS-in-JS approach though, you'll likely be more susceptible to a flash of unstyled content. (FOUC), so will need to comment that YMMV - tracked in Roadmap To 1.0 And Beyond #418

@thescientist13 thescientist13 added bug Something isn't working website Tasks related to the projects website / documentation documentation Greenwood specific docs labels Dec 24, 2020
@thescientist13 thescientist13 added the P0 Critical issue that should get addressed ASAP label Dec 24, 2020
@thescientist13 thescientist13 changed the title resolve multiple slot rendering on the home page resolve multiple <slot> elements rendering on the home page Dec 24, 2020
@thescientist13 thescientist13 self-assigned this Dec 24, 2020
@thescientist13 thescientist13 force-pushed the bug/issue-433-resolve-slot-double-rendering branch from ee60fe0 to 9b055a3 Compare December 30, 2020 17:28
@thescientist13 thescientist13 marked this pull request as ready for review December 30, 2020 17:28
@thescientist13
Copy link
Member Author

@aholtzman
If you have a little time, could use a little help with style tweaks here to restore / cleanup the card styles given the new DOM structure we need to support our usage of <slot> here to match what it looked like before. 🙏

@thescientist13 thescientist13 added the help wanted Extra attention is needed label Dec 30, 2020
@thescientist13 thescientist13 removed their assignment Dec 30, 2020
${this.renderImage()}
${this.renderTitle()}
<slot name="cardcontent"></slot>
<div class="gwd-card">
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately this wont work for yarn serve because this moves the <slot> away from being a direct descendent of the :host 😞

@thescientist13
Copy link
Member Author

Something else i've come to realize about this issue is that when we pre-render a web component, the final HTML is not just the tags

<my-component></my-component>

but the tags plus whatever its contents render out to, e.g.

<my-component>
  <header>
    <h1>Lorum Ipsum</h1>
    <!-- etc -->
  </header>
</my-component>

The issue being that this pre-rendered markup is light DOM. When a web component mounts, it will by default attach a ShadowDOM. This means it will have both light and shadow DOM. This becomes an issue now because the light dom will now compete with the content projected into the Shadow DOM via <slot> because effectively that pre-rendered content will be slot content.

So short term there are some best practices and "rules" we can document for when <slot> is used which I have done. I have some ideas for how we can optimize for this case without having to burden the user. Let me fix the styles here first though and will track this for a future issue.

@thescientist13 thescientist13 added help wanted Extra attention is needed and removed help wanted Extra attention is needed labels Jan 21, 2021
@thescientist13 thescientist13 removed the help wanted Extra attention is needed label Jan 21, 2021
@thescientist13 thescientist13 removed their assignment Jan 21, 2021
@thescientist13 thescientist13 linked an issue Jan 21, 2021 that may be closed by this pull request
5 tasks
@thescientist13 thescientist13 merged commit 4972552 into release/0.10.0 Jan 23, 2021
@thescientist13 thescientist13 deleted the bug/issue-433-resolve-slot-double-rendering branch January 23, 2021 17:37
thescientist13 added a commit that referenced this pull request Apr 3, 2021
* initial WIP to resolve the issue

* example code cleanup

* supplemental documentation for Shadow DOM and slot usage

* restore card padding

* fix padding, revert slot to direct decendent of host

* remove extra dependency

* home page card style tweaks

Co-authored-by: aholtzman <[email protected]>
thescientist13 added a commit that referenced this pull request Apr 3, 2021
* initial WIP to resolve the issue

* example code cleanup

* supplemental documentation for Shadow DOM and slot usage

* restore card padding

* fix padding, revert slot to direct decendent of host

* remove extra dependency

* home page card style tweaks

Co-authored-by: aholtzman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Greenwood specific docs P0 Critical issue that should get addressed ASAP website Tasks related to the projects website / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

instances of <slot>-ed content and lit-element are rendering twice
3 participants