-
-
Notifications
You must be signed in to change notification settings - Fork 19
[Wip][Feature] Add teleport helper #42
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
[Wip][Feature] Add teleport helper #42
Conversation
Is the teleported item still reactive? <div x-data="{foo: 'bar'}" x-init="$teleport('target', $refs.item)">
<div x-ref="item" x-text="foo"></div>
<button @click="foo = 'baz'">click me</button>
</div>
<div x-portal="target"></div> Does it work? Another question? Do we need x-portal? It doesn't do anything, it could just be an id rather than an unofficial directive which could conflict one day. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is still a WIP but these are the only questions I've got 😄
src/teleport.js
Outdated
|
||
// throw error if portal is undefined | ||
if (destination === undefined) { | ||
throw Error('Undefined portal: ', destination) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throwing error vs console.warn-ing?
Also it might be better to have the message say
throw Error('Undefined portal: ', destination) | |
throw Error(`Portal "${target}" not found, got "${destination}", does your element have the x-portal="${target}" attribute?`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$teleport not defaulting to body
if target
is provided. So warning didn't make sense to me. But we can change if all agrees.
about message yeah, i agree. Will change now.
src/teleport.js
Outdated
} | ||
// append element to portal destination | ||
destination.append($element) | ||
}, 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 this 100
makes my race-condition worry-meter go off, could we poll until __x
is defined instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i am aimed to find out how to use it as
$el.__x.nextTickStack.push(() => destination.append($element))
that's where i am stuck at the moment. Any suggestion will be helpful. $el.__x is undefined in helpers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do x-init="$teleport(.. )"
Alpine will still be building the component so __x won't be available and your directives won't be evaluated yet.
I think you won't need a timeout if you do x-init="() => $teleport(...)"
. Maybe we can just make clear in the docs that it needs to run after alpine is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that works, but we still need a way to access $el.__x
for the fully supported reactivity. I believe if i can inject the append in nextTickStack
, communication between old and new place will work both ways.
|
That's some sort of unexpected magic, cool. About x-portal, it's just me being a bit fussy. It's not standard and it doesn't really do anything, it's just an html attribute so target may as well be a normal css selector. I'm not too precious though if others like it. |
Sorry, I just noticed. it is working opposite way. If I strongly believe if we can find out how to make |
Or even the DOM node/element as a target (output of querySelector for example) |
Did work on this a bit, i am happy with this: const target = typeof destination === 'string' ? document.querySelector(destination) : destination
//tag
<div x-init="() => $teleport('div')">
//id
<div x-init="() => $teleport('#destination')">
//class
<div x-init="() => $teleport('.destination')">
//atribute
<div x-init="() => $teleport('[destination]')">
//reference
<div x-init="() => $teleport($refs.destination)"> |
at this point though, surely people could write |
Yes, this will work as well. |
src/teleport.js
Outdated
checkForAlpine() | ||
Alpine.addMagicProperty('teleport', ($el) => { | ||
return function (destination = document.body, $element = $el, prepend = false) { | ||
// find out destination |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will want to follow a pattern on adding options as an object, especially when it's a boolean. So it would be $teleport(from, to, options)
in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i think we can improve this part. Thanks for the suggestion.
Could you make this work with Also, it feels like we shouldn't be breaking apart Alpine components. I feel like this is going to be misunderstood and misused and just cause a lot of people confusion. Maybe better to limit it to only transferring the entire component? Some weird behavior here with instructions at the end: https://codepen.io/KevinBatdorf/pen/bac92ed14ffeb8b1ce0e47f949b72433?editors=1000 I think if this helper were to only move a component from one place to another (for example into a modal), and save its state, then it will be useful. What other use cases do you have in mind? |
Could this also do something like this? <div x-data @click="$teleport('#modal', `<div x-data=\"{ foo:'bar' }></div>\")"></div> Does that feel like something that fits here? |
This should have probably also been open first as a discussion by the way :) Lot's of questions. Other places call this a portal right? Any reason not to use |
I don't think this will work. We need string or a node object to be passed. Also why would you write html with javascript on DOM ? :) |
Yes, i think we can safely move to Discussion now. |
Just a thought really, but it could solve a few common requests people have. Mainly everyone wants a way to abstract partials without much effort. The HTML could come from $( ".inner" ).prepend( "<p>Test</p>" ) We could have Alpine components that act as templates and can be "teleported" anywhere on demand. Might work better in a different helper though. I'll put some thought into it. |
@KevinBatdorf interval test failing for some reason. Also sometimes it happens on my local. Probably there is something related with timeouts |
I'll have to come back to that at a later time. I'll open an issue though. I think you can just re-run it and it will pass. i've noticed it once before as well. |
Probably not a good fit here. Closing this for now. |
This PR adds teleport helper
$el
,$refs.anything
,'string'
$(target = document.body, $element = $el, prepend = false)
** Example**
Demo