improve `<Portal/>` container, add 2 new properties

[ngdangtu-vn]

I don't know what is "three mandatory fields" since I only saw 2 headings. But I filled all of them.

Summary

improve `<Portal/>` container, add 2 new properties

- `asElement` allows to change Portal container element type
- `class` allows to set class for Portal container

Bascially, I don't like a bare <div/> wrap children component so I would like to suggest make Portal container be more customisable with tagName and class. We can find a way to support dataset later if it requires.

How did you test this change?

I've update test file and run test script from package.jsons:

1. / → pnpm test
2. cd ./packages/solid → pnpm test

Both of them return all pass results so I assume there is nothing would break the framework. As for testing on real field, I'm not sure how to do that yet.

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 0475ac2bc46ac787a444d09291e64cc0797b0cfd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[mizulu]

I wonder, if defaulting the container with this inline style will solve most issues that will require a custom element

it is possible to hack it as shown here https://github.com/solidjs/solid/issues/1873#issuecomment-1875092507

if Portal has intentions to support fragments in the future ( no wrapper ) then these might not be necessary. ( as might have been implied by https://github.com/solidjs/solid/issues/1873#issuecomment-2623761894)

it is nice however to have control of the container element however and its styling. with the current implementation of Portal. without resorting to the implementation details.

[ngdangtu-vn]

I'm not sure I would want a hacky solution. It is not friendly with accessibility since div and article can be meant different thing. For now, without this pr, we can use your suggestion set aria-*= and role= on it. But I prefer to have native html tag more.

Fragment support is nice though.

[mizulu]

one more thing You have added class, but what if I have a different requirement maybe I need to set style, or some other attribute, am I to resort to use the ref, which we consider hacky?

[ngdangtu-vn]

Yeah, when I made this, I didn't think that far. But you are right. It should support all html attributes. We can do that with setAtrribute. But how should handle it in the Portal component property? Put all of them in one propety like containerAttributes or give them first level property like class?

@mizulu what's your thought?

[mizulu]

Yeah, when I made this, I didn't think that far. But you are right. It should support all html attributes. We can do that with setAtrribute. But how should handle it in the Portal component property? Put all of them in one propety like containerAttributes or give them first level property like class?

first of all before you make any more changes, I would wait for @ryansolid feedback. so no effort is wasted, in case there is no intention to make portal changes.

now back to the discussion if Portal is "recognized" as wrapping element, then all the props and attributes could just be delegated

<Portal 
          element={()=>{<div/>}} 
          fallback={()=>{}}  
          style={} class={} attr:id={"portal_1"}  prop:tabIndex={0}>
</Portal>

but if we do it this way, I don't know if Portal will be able to narrow its type to HTMLDivElement it could do it for Components like <Dynamic> does.

and if we already pass an element, perhaps the better api for this will simply be

<Portal 
          element={()=><div class={myclass} attr:id="portal_1" />}
          fallback={()=>{}}>
</Portal>

some more considerations:

1. make sure ref still works
2. should or could the element by dynamic

[ryansolid]

The truth is we are redesigning Portal's in 2.0. I figured out a way to use proxies to proxy DOM elements that will allow us to get rid of the intermediate DOM element. So while I know it is a bit clunky right now I'm less incentivized to change the API to encourage patterns that will be going away anyway. For now messing with the ref is probably the way to go.

[mizulu]

@ryansolid is the new Portal going to be back porteled to solid 1.0?

[ryansolid]

@ryansolid is the new Portal going to be back porteled to solid 1.0?

No because it is a breaking change. It could be implementable but I can't change the existing Portal that way.

[mizulu]

If there is no plan to backport, then maybe we should allow the user to control the wrapper container as suggested above

<Portal 
          element={()=><div class={myclass} attr:id="portal_1" />} >
</Portal>

or more simply just pass the constructor string div p, etc combined with the ref for additional control

<Portal 
          element="p" >

using ref is already an awkward pattern , which is going to be "broken" in v2.0

allowing this will remove one more barrier from users who are happy with solid 1.0 and may not move to 2.0

we can consider enabling fragmented portals in solid 1.0 by flag, as to not make it a breaking change.

<Portal element={<></>}/>
or 
<Portal fragment />

[Perfumere]

Need <Fragment> functionality, please consider not forcing the use of DOM element wrapping.

[Perfumere]

I have a temp custom component: Teleport，no div wrapper:

use case

// 1. default to document.body
<Teleport when={mounted()}></Template>

// 2. cowork with Transition
const [show, setShow] = createSignal(false);
const [mounted, setMounted] = createSignal(false);
<Teleport when={mounted()}>
        <Transition name="fade-in" appear onAfterExit={() => setMounted(false)}>
        { show() && <div>Toast...</div> }
        </Transition>
<Teleport>

// 3. modify `props.mount`，move props.children from  `old mount` to `new mount`
const [el, setEl] = createSignal(document.body);
setTimeout(() => setEl(document.documentElement), 5000); // change mount after 5s.
<Teleport mount={el}>
  <For each={[0, 1,2,3]}>{i => i}</For>
  Anytype...
</Teleport>

Source Code

import {
  createEffect,
  createRenderEffect,
  createRoot,
  getOwner,
  runWithOwner,
} from 'solid-js';
import { insert } from 'solid-js/web';
import type { JSX, Accessor } from 'solid-js';

type TeleportProps = {
  mount?: HTMLElement | null;
  when?: boolean | Accessor<boolean>;
  children: JSX.Element;
};

export function Teleport(props: TeleportProps) {
  const owner = getOwner();
  let parentEl: HTMLElement | null = null;
  let disposeCurrent: (() => void) | null = null;
  let baselineTail: ChildNode | null = null;
  let managedFirst: ChildNode | null = null;
  let managedLast: ChildNode | null = null;

  const disposeRoot = () => {
    if (disposeCurrent) {
      disposeCurrent();
      disposeCurrent = null;
    }
  };

  const updateManagedBounds = () => {
    if (!parentEl) return;
    const first = baselineTail ? baselineTail.nextSibling : parentEl.firstChild;
    const last = parentEl.lastChild;
    if (!first || !last || first === baselineTail) {
      managedFirst = null;
      managedLast = null;
      return;
    }
    managedFirst = first;
    managedLast = last;
  };

  const mountInto = (target: HTMLElement) => {
    parentEl = target;
    baselineTail = parentEl.lastChild;
    runWithOwner(owner, () => {
      createRoot(dispose => {
        disposeCurrent = dispose;
        createRenderEffect(() => {
          insert(parentEl as Node, () => props.children, null);
          updateManagedBounds();
        });
        return undefined as unknown as void;
      });
    });
  };

  const moveManagedTo = (nextParent: HTMLElement) => {
    if (!parentEl || parentEl === nextParent) return;
    const newBaseline = nextParent.lastChild;
    if (!managedFirst || !managedLast) {
      parentEl = nextParent;
      baselineTail = newBaseline;
      updateManagedBounds();
      return;
    }
    let node: ChildNode | null = managedFirst;
    const lastToMove: ChildNode = managedLast;
    while (node) {
      const next = node === lastToMove ? null : node.nextSibling;
      nextParent.appendChild(node);
      if (node === lastToMove) break;
      node = next;
    }
    parentEl = nextParent;
    baselineTail = newBaseline;
    // remark start & end
    managedFirst = newBaseline ? (newBaseline.nextSibling as ChildNode | null) : (nextParent.firstChild as ChildNode | null);
    managedLast = lastToMove;
  };

  const cleanupManaged = () => {
    if (parentEl && managedFirst && managedLast) {
      let node: ChildNode | null = managedFirst;
      const lastToRemove: ChildNode = managedLast;
      while (node) {
        const next = node === lastToRemove ? null : node.nextSibling;
        parentEl.removeChild(node);
        if (node === lastToRemove) break;
        node = next;
      }
    }
    managedFirst = null;
    managedLast = null;
    baselineTail = null;
    parentEl = null;
  };

  createEffect(() => {
    const shouldRender = props.when ?? true;
    const target = (props.mount || document.body) as HTMLElement;

    if (!shouldRender) {
      disposeRoot();
      cleanupManaged();
      return;
    }

    if (parentEl && parentEl !== target) {
      moveManagedTo(target);
    } else if (!parentEl) {
      mountInto(target);
    } else if (!disposeCurrent) {
      mountInto(target);
    }
  });

  onCleanup(() => {
    disposeRoot();
    cleanupManaged();
  });

  return null;
}