Bump leptos dependency to 0.5. #4
Conversation
src/portal.rs
Outdated
| somewhere near the root of the app"; | ||
|
|
||
| #[derive(Clone)] | ||
| struct PortalCtx(StoredValue<Vec<(TypeId, RwSignal<Option<Children>>)>>); | ||
| struct PortalCtx(StoredValue<Vec<(TypeId, RwSignal<Option<ChildrenFn>>)>>); |
There was a problem hiding this comment.
This was a change intentionally made. It would be better if we can use Children instead of ChildrenFn. What's the reason we need ChildrenFn again?
There was a problem hiding this comment.
It doesn't compile with Children, likely due to the changes in the ownership system, but I must admit I didn't investigate further.
There was a problem hiding this comment.
Ahh, got it. So one of the reasons why we changed it to Children was because a lot of times, in custom components, we take in Children as a prop, and pass that onto the portal component. If we can retain that, that would be amazing
There was a problem hiding this comment.
Children can only be called once, because it's type alias is FnOnce() -> View, whereas ChildrenFn is Fn() -> View. This means that if you use Children, you can only ever call the children function exactly once. If you need children to render more than once, such as when moving into an Fn() for DynChild, then you must use ChildrenFn, because it lets you call the children
There was a problem hiding this comment.
Can we perhaps use FnOnce() + Clone then?
I remember having significant issues with using ChildrenFn instead of Children, which is why the change was made.
Alternatively, can we perhaps add an example / test for making a custom component on top of the portal so that we can make sure it compiles?
There was a problem hiding this comment.
I haven't yet reviewed the PR, but it should be possible to use Children without requiring Clone...that's what the Option<Children> is for...when the children changes, we can children.take() and then render the FnOnce() -> View.
|
Thank you for making this PR! I really appreciate it. Could you please bump it to leptos |
Will do |
|
Hey. Now that leptos 0.5 stable is out, we can update the dependency I suppose? Once that is done, @jquesada2016 would you happen to have the time to make a release? |
|
@rakshith-ravi yes of course! Now, I have two options. I am wanting to rewrite the components to use the |
|
I think we can make a release so that we at least have something to work with in the meantime, and then make another release once we're reworked the internals. Also, I noticed that @maccesch has put up a PR upstream at leptos-rs/leptos#1820. An alternate idea would be to release what we have now, and then collaborate with him to get the |
|
My approach is a little different. I really like how Solid does the Portal. There's no target or context needed. Just an optional |
Would that allow for multiple portals? Like nested-portals kinda stuff |
| leptos = { version = "0.5.0", features = ["csr"] } |
There was a problem hiding this comment.
Can we keep this to 0.5 instead of specifically using 0.5.0?
|
On second thought, I really don't know if anybody would even want something like that, so idk |
|
I haven't tried it but I don't see why not. |
|
I have updated the branch to Looking at the code, though, there is something that escapes me... We are using |
|
This definitely doesn't work with the |
|
Why |
|
@rakshith-ravi, that's less restrictive than |
|
So, I haven't tried with |
|
Looks like @maccesch's version of portal is merged upstream. (leptos-rs/leptos#1820) Would that solve the problem for us? |
This adds support for leptos 0.5 rc1.
Note that
Portalnow needsChildrenFninstead ofChildren. Everything else is unchanged except for the API change introduced by leptos-rs/leptos#918