Skip to content
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

Replace ember-wormhole with ember-maybe-in-element #319

Closed
wants to merge 1 commit into from

Conversation

maxfierke
Copy link
Collaborator

@maxfierke maxfierke commented Nov 17, 2018

ember-maybe-in-element leverages the Glimmer native -in-element
helper to render content into another DOM node. This allows us to
drop ember-wormhole and may address the issues raised in #309.

ember-maybe-in-element leverages the Glimmer native
helper to render content into another DOM node. This allows us to
drop ember-wormhole and may address the issues raised in #309.
@maxfierke maxfierke mentioned this pull request Nov 17, 2018
@maxfierke maxfierke added this to the 3.1.0 milestone Nov 19, 2018
@@ -160,6 +160,14 @@ export default Component.extend({
return `${this.get('elementId')}-wormhole`;
}),

wormholeEl: computed('wormholeId', '_awaitingTooltipElementRendered', function() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is wormhole a misnomer now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on how you look at it, I guess. Conceptually, it's still a bit of a wormhole.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would maybe be portal? I think that's what React calls their wormholes.

@joelluijmes
Copy link

Please see my comment on the issue #309 (comment), this actually breaks the tooltip in my case.

@maxfierke maxfierke removed this from the 3.1.0 milestone Nov 26, 2018
@maxfierke
Copy link
Collaborator Author

Going to close this for now. I think it's definitely something to look into later, but also might be better off implementing something ourselves with -in-element directly and skipping the dependency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants