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

Revert "Fixes rootElement to match that of this.$()[0] (#121)" #132

Merged
merged 1 commit into from
Dec 8, 2017

Conversation

cibernox
Copy link
Owner

@cibernox cibernox commented Dec 8, 2017

This reverts commit 6300642.

@cibernox cibernox merged commit 58b1dfe into master Dec 8, 2017
@cibernox cibernox deleted the revert-problematic-change branch December 8, 2017 17:36
@Turbo87
Copy link
Contributor

Turbo87 commented Dec 12, 2017

is there a discussion somewhere that discusses why this was reverted?

@pixelhandler
Copy link
Collaborator

pixelhandler commented Dec 12, 2017

@Turbo87 my guess is that there was not a good way to over-ride what the addon sets as the root element, e.g. config sets rootElement: '#ember-testing' or something different, but the addon wants to use the first child as the root to match this.$()[0] which is rootElement: '#ember-testing > .ember-view'

Perhaps this RFC addresses a better solution, emberjs/rfcs#280 rendered

@cibernox
Copy link
Owner Author

I reverted it mostly because it was causing issues in addons that used ember-basic-dropdown, like EPS. The reason is that, in testing, the target of the wormhole is #ember-testing, but since this change the root was #ember-testing > .ember-view.
That cause that find/findAll could not find wormholed content.

I still think that this should be done, but I wanted to adapt ember-basic-dropdown first.
For that I made a fix in ember-wormhole, but it's not yet enough, because we can't reference wormhole destination by complex selectors, it only accepts IDs.

I proposed to add querySelector/querySelectorAll to simple-dom (ember-fastboot/simple-dom#53) but I don't have a response yet.

And on top of that, yesterday I made the RFC that proposes to remove the .ember-view wrapper on tests, so this might not even be necessary soon.

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.

3 participants