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