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

[Glimmer2] Components should have access to the view registry #13505

Closed
wants to merge 1 commit into from

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
Sponsor 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