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]

⚠️ No Changeset found

Latest commit: 0475ac2

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
#1873 (comment)

if Portal has intentions to support fragments in the future ( no wrapper )
then these might not be necessary. ( as might have been implied by #1873 (comment))

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 />