Skip to content

Conversation

@chadhietala
Copy link
Contributor

@chadhietala chadhietala commented May 16, 2016

Not sure if this is something we want to continue with or not
/cc @wycats @chancancode

@krisselden
Copy link
Contributor

This is fine for now but this test is testing something internal more HOW instead of WHAT, I would like to move the view registry to the renderer or event dispatcher (which in reality should be per document rather than per app).

@chadhietala
Copy link
Contributor Author

@krisselden Gotcha. I think I was more or less asking if this is the right place. In the current implementation this gets set in a hook that I don't think exists in glimmer https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/mixins/view_child_views_support.js#L83-L126

@krisselden
Copy link
Contributor

I'm surprised that is even used by anything, that has more to do with the old view layer than anything else

@mixonic
Copy link
Member

mixonic commented Jun 4, 2016

@chadhietala should it also be set onto the instance in init for shaping perhaps?

@krisselden
Copy link
Contributor

@mixonic seems like this could just be injected onto the event dispatcher, I don't understand why every component needs this field?

@rwjblue
Copy link
Member

rwjblue commented Jun 7, 2016

Today, the views register themselves upon init. This could easily be done by the renderer or other higher level environment thing though.

Ultimately, I don't think we have to inject this into each component. The key bit is that the registry must exist and been kept up to date.

@homu
Copy link
Contributor

homu commented Jun 27, 2016

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

@krisselden
Copy link
Contributor

superseded by #13852 which handles event dispatching without needing the behavior this test was testing.

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.

5 participants