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

Container view hooks #1198

Merged
merged 1 commit into from Oct 15, 2012

Conversation

Projects
None yet
5 participants
@tchak
Member

tchak commented Jul 20, 2012

It helps in some cases where we need to implement some special behavior
on outlet connection.
We add four new methods to containerView :
presentCurrentView and dismissCurrentView
appendCurrentView and removeCurrentView

In cases where we need some special handeling like animations you can
override theus methods

Add willAppear / willDisappear and didAppear / didDisappear

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 20, 2012

This pull request passes (merged 52534676 into 2d9f107).

travisbot commented Jul 20, 2012

This pull request passes (merged 52534676 into 2d9f107).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 21, 2012

This pull request passes (merged bacb9b72 into a567b35).

travisbot commented Jul 21, 2012

This pull request passes (merged bacb9b72 into a567b35).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 22, 2012

This pull request passes (merged 1943b345 into 07141fd).

travisbot commented Jul 22, 2012

This pull request passes (merged 1943b345 into 07141fd).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 22, 2012

This pull request passes (merged b92fddf3 into 07141fd).

travisbot commented Jul 22, 2012

This pull request passes (merged b92fddf3 into 07141fd).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 22, 2012

This pull request passes (merged 85a68f8e into 07141fd).

travisbot commented Jul 22, 2012

This pull request passes (merged 85a68f8e into 07141fd).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 24, 2012

This pull request passes (merged 7130c221 into 313d2ef).

travisbot commented Jul 24, 2012

This pull request passes (merged 7130c221 into 313d2ef).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 27, 2012

This pull request passes (merged a85ccb92 into d7e24ac).

travisbot commented Jul 27, 2012

This pull request passes (merged a85ccb92 into d7e24ac).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 15, 2012

This pull request passes (merged e71fd70d into a2caaa3).

travisbot commented Aug 15, 2012

This pull request passes (merged e71fd70d into a2caaa3).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 15, 2012

This pull request passes (merged 05f4d37c into a2caaa3).

travisbot commented Aug 15, 2012

This pull request passes (merged 05f4d37c into a2caaa3).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 15, 2012

This pull request passes (merged 2534d705 into 138fbdf).

travisbot commented Aug 15, 2012

This pull request passes (merged 2534d705 into 138fbdf).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 19, 2012

This pull request passes (merged 99cb9d38 into 9713824).

travisbot commented Aug 19, 2012

This pull request passes (merged 99cb9d38 into 9713824).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 21, 2012

This pull request passes (merged 0a5219e4 into 3f7a118).

travisbot commented Aug 21, 2012

This pull request passes (merged 0a5219e4 into 3f7a118).

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Aug 21, 2012

Member

@kselden I added a test and more docs, could you review this?

Member

tchak commented Aug 21, 2012

@kselden I added a test and more docs, could you review this?

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 21, 2012

This pull request passes (merged adbea4b3 into 3f7a118).

travisbot commented Aug 21, 2012

This pull request passes (merged adbea4b3 into 3f7a118).

if (currentView) {
childViews.pushObject(currentView);
set(currentView, 'isBeingPresented', true);

This comment has been minimized.

@krisselden

krisselden Aug 21, 2012

Member

why is this needed in addition to the willAppear/didAppear events?

@krisselden

krisselden Aug 21, 2012

Member

why is this needed in addition to the willAppear/didAppear events?

This comment has been minimized.

@tchak

tchak Aug 22, 2012

Member

I thought some one could want do in a template

{{#if view.isBeingPresented}}
  I am entering the screen!
{{/if}}
@tchak

tchak Aug 22, 2012

Member

I thought some one could want do in a template

{{#if view.isBeingPresented}}
  I am entering the screen!
{{/if}}
Add hooks to containerView.
It helps in some cases where we need to implement some special behavior
on outlet connection.
We add four new methods to containerView :
`presentCurrentView` and `dismissCurrentView`
`appendCurrentView` and `removeCurrentView`

In cases where we need some special handeling like animations you can
override theus methods

Add willAppear / willDisappear and didAppear / didDisappear

Refactor so it would be easier to override presentCurrentView / dismissCurrentView
@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Oct 8, 2012

Member

I think this commit is basically waiting on us to decide whether we want to support this behavior.

Member

wagenet commented Oct 8, 2012

I think this commit is basically waiting on us to decide whether we want to support this behavior.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Oct 8, 2012

Member

@wagenet yes, and if not we need to provide an alternative to achieve something similar.
@kselden proposed to generalize the hooks to any childView insertion. It is probably doable, but will introduce a fair load of complexity in the view states layer. If you decide it should be done, I can work on it.

Member

tchak commented Oct 8, 2012

@wagenet yes, and if not we need to provide an alternative to achieve something similar.
@kselden proposed to generalize the hooks to any childView insertion. It is probably doable, but will introduce a fair load of complexity in the view states layer. If you decide it should be done, I can work on it.

wycats added a commit that referenced this pull request Oct 15, 2012

@wycats wycats merged commit 5538ab1 into emberjs:master Oct 15, 2012

1 check passed

default The Travis build passed
Details
@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Oct 23, 2012

Member

I'm running into issues in my app with currentView already being destroyed at this point. Trying to set on a destroyed object causes things to blow up. It's possible that my app is in a bad state, but if not, we need to fix this.

Member

wagenet commented on packages/ember-views/lib/views/container_view.js in a2cd03b Oct 23, 2012

I'm running into issues in my app with currentView already being destroyed at this point. Trying to set on a destroyed object causes things to blow up. It's possible that my app is in a bad state, but if not, we need to fix this.

wagenet added a commit that referenced this pull request Oct 23, 2012

Revert "Merge pull request #1198 from tchak/container-view-hooks"
This reverts commit 5538ab1, reversing
changes made to 305202f.

This change was reverted since the API changes didn't undergo sufficient
discussion. We hope to merge something similar in the future.

Conflicts:
	packages/ember-views/lib/views/container_view.js
@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Oct 23, 2012

Member

Paul, after discussion we decided to revert this commit in favor of a formal proposal and more discussion. Sorry for the hassle, we definitely do want something like this.

Member

wagenet commented Oct 23, 2012

Paul, after discussion we decided to revert this commit in favor of a formal proposal and more discussion. Sorry for the hassle, we definitely do want something like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment