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

[WIP] [FEAT] converts -in-element to -render-portal #14472

Closed
wants to merge 1 commit into from

Conversation

runspired
Copy link
Contributor

Tracking PR, requires bumping glimmer first, still figuring out why tests are failing locally.

@mixonic
Copy link
Member

mixonic commented Oct 17, 2016

@runspired can you provide some context and/or links to other issues related to this?

@mixonic
Copy link
Member

mixonic commented Oct 17, 2016

Looks like glimmerjs/glimmer-vm#334 is the upstream item related to this.

@krisselden
Copy link
Contributor

This should be relabeled as a FEATURE

@mitchlloyd
Copy link
Contributor

mitchlloyd commented Nov 6, 2016

@runspired I was curious about using -in-element in Ember Islands, but first wanted to understand more about the deficiencies you listed in the related issue:

in-element deficiencies

  • no support for renderInPlace
  • no ability to move the DOM from one remote target to another remote target or from a remote target to in-place.

I spiked a change to Ember Wormhole using -in-element to learn more: mitchlloyd/ember-wormhole@a70eea5

So said another way, it seems the issue is that Ember Wormhole guarantees that the yielded DOM will remain stable when changing the destinationId whereas -in-element does not. For 99% of use cases this is probably fine, but it's definitely a breaking change. Awesome that you're taking this on!

The wormhole concept has often been referred to as a "portal" or "portal-ing" in other frameworks and in talks. I'm also particularly partial to the game Portal.

I wanted to weigh in with a strong preference for the name in-element over render-portal. in-element might not be as fun as render-portal but {{#in-element sidebar}}... expresses the behavior better than {{#render-portal sidebar}}.... If we are leaning toward render-portal then I'd like to suggest render-portal-2. Most critics agree that that although Portal was innovative, Portal 2 is a stronger game overall 😄

@runspired runspired changed the title [WIP] [BUGFIX beta] converts -in-element to -render-portal [WIP] [FEAT] converts -in-element to -render-portal Nov 6, 2016
@runspired
Copy link
Contributor Author

@mitchlloyd

If we are leaning toward render-portal then I'd like to suggest render-portal-2. Most critics agree that that although Portal was innovative, Portal 2 is a stronger game overall

LOL

I wanted to weigh in with a strong preference for the name in-element over render-portal

so @rwjblue's objection to in-element once my proposed changes were in place was more or less "but if you can render-in-place this name makes no sense", which I sort-of-ish agree with.

@mitchlloyd
Copy link
Contributor

I'll try to save any more of my bike-shedding for an RFC, but I do want to clarify:

but if you can render-in-place this name makes no sense

It looks like this change is all about making sure the DOM stays stable and there is nothing specifically about "rendering in place". I believe these assertions show the primary issue you're solving: https://github.com/tildeio/glimmer/pull/334/files#diff-793e7c01665aa0549d057403a3859c15R256. Is that true or am I missing something?

@runspired
Copy link
Contributor Author

runspired commented Nov 6, 2016

@mitchlloyd if you look at those assertions you will notice that when the element is null the assertion always tests that it was rendered in place :P (which is also a requirement of wormhole)

(this is also discussed in the conversation for that PR)

@pixelhandler
Copy link
Contributor

@runspired will this need feature flags as well?

@homu
Copy link
Contributor

homu commented Nov 26, 2016

☔ The latest upstream changes (presumably #14633) made this pull request unmergeable. Please resolve the merge conflicts.

@mmun
Copy link
Member

mmun commented Feb 21, 2018

I believe this should be closed as the discussion has moved to emberjs/rfcs#287. Let me know if I missed something and need to reopen.

@mmun mmun closed this Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants