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

[#1976] Add support for the replaceElement option on Region #1980

Closed
wants to merge 1 commit into from
Closed

[#1976] Add support for the replaceElement option on Region #1980

wants to merge 1 commit into from

Conversation

trezy
Copy link
Member

@trezy trezy commented Oct 8, 2014

NOTE: I'll try to write the specs this evening and push them up. I just wanted to get the code in front of you guys for further discussion.

I'll reiterate my approach from the #1976:

  1. Instead of changing the value of the region.el, I'm simply replacing the element in the DOM with region.replaceEl() and region.restoreEl(). region.el stays in tact.
  2. On region.show( view, { replaceElement: true }) I am setting region.replaced. This allows all future executions on region to determine if they need to address region.el or view.el.
  3. On region.empty() I am checking the value of region.replaced. If it's true, I'll replace the view.el with region.el before destroying the view. This way any future executions on region.el will refer to the correct node.

TO DO

  • I'm still not sure about handling this when a view is destroyed from outside of the region. It may just magically work if I'm super lucky. I'll test it later and hopefully come up with solution if necessary after I write the primary specs.
  • replaceEl() and restoreEl() can probably be abstracted just a little more but I don't know that it's really necessary.
  • A global configuration option to make this a default behavior would be nice. Again, I'm not sure that it's necessary. Even if it is it's probably worthy of a separate issue.

This needs WAY more testing. I only beat on it with region.show(), region.empty(), and region.reset().

@trezy trezy changed the title Add support for the replaceElement option on Region [#1976] Add support for the replaceElement option on Region Oct 8, 2014
@samccone
Copy link
Member

samccone commented Oct 8, 2014

woo cool @trezy


just a side note... adding more code to regions at this time imho is a debt.
region is already the most foobar class that we have and making it do even more is going to hurt us in ways that I am SURE we are not aware of.

If our plans play out v3 being the region fix release... then this work seems like it will fit in nicely with the new region class....

@jamesplease
Copy link
Member

If our plans play out v3 being the region fix release... then this work seems like it will fit in nicely with the new region class....

Agreed.

I like this idea. This PR shows that it's simple enough to implement, but I think it's wise for us to wait for v3.

@trezy
Copy link
Member Author

trezy commented Oct 8, 2014

Woohoo! You guys like my stuff!

...

I don't have anything helpful to add, I'm just excited. 😃

@jamesplease
Copy link
Member

I'm going to close this and add it to the v3 doc over in #1796. It won't land in 2.x, but it will be in v3.

@jamesplease jamesplease added this to the v3.0.0 milestone Oct 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants