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

Switch to yield the select to the block #45

Closed
wants to merge 2 commits into from
Closed

Switch to yield the select to the block #45

wants to merge 2 commits into from

Conversation

tpitale
Copy link

@tpitale tpitale commented Jul 11, 2015

  • accept select on x-option
  • use select on x-option when register
  • fallback to nearestNeighbor for non-ember-2 beta
  • yield select to the block in handlebars
  • pass the select for blockless version

* accept select on x-option
* use select on x-option when register
* fallback to nearestNeighbor for non-ember-2 beta
* yield select to the block in handlebars
* pass the select for blockless version
@tpitale
Copy link
Author

tpitale commented Jul 11, 2015

Attempt to resolve #44.

@@ -55,9 +64,9 @@ export default Ember.Component.extend({
this._super.apply(this, arguments);

Ember.run.scheduleOnce('afterRender', () => {
var select = this.nearestOfType(XSelectComponent);
/* Use an assigned select, or search for one nearby */
var select = this.get('select') || this.nearestOfType(XSelectComponent);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still check for nearestOfType to try to avoid breaking anyone that hasn't upgrade to a version of ember that breaks it anyway.

@Robdel12
Copy link
Collaborator

I'm going to go ahead and close this since this bug is actually an ember bug. It should be fixed in 1.13.4. If not we'll revisit :D

@Robdel12 Robdel12 closed this Jul 20, 2015
@tpitale
Copy link
Author

tpitale commented Jul 21, 2015

This bugfix is gone in ember 2 beta. I believe it was intentionally removed. So, this will probably be broken again shortly.

This PR supports both the explicit yield, and the old syntax in 1.13.4. And it appears to be the way ember wants components to move.

@rwjblue
Copy link
Member

rwjblue commented Jul 21, 2015

This bugfix is gone in ember 2 beta.

Can you point me to what you mean? nearestOfType should continue to work in 2.0.0-beta.3...

@Robdel12
Copy link
Collaborator

I'm also unsure how well the addon works in the beta right now. We need to upgrade ember-mocha to see where our test suite is at :)

@tpitale
Copy link
Author

tpitale commented Jul 21, 2015

I'll try beta 3. It just appeared that the conversation was saying the code is or would soon be deprecated. I'm mostly concerned because I can't find the bug fix from 1.13.4 in master in nearestOfType.

@tpitale
Copy link
Author

tpitale commented Jul 21, 2015

emberjs/ember.js#11170 That conversation.

@cowboyd
Copy link
Collaborator

cowboyd commented Jul 21, 2015

FWIW, I actually would prefer to not use nearestOfType, and once we can yield component helpers as block params it would be unecessary. Until then, it does provide the nicest api to the user.

@rwjblue
Copy link
Member

rwjblue commented Jul 21, 2015

From emberjs/ember.js#11170 (comment):

We decided not to deprecate accessing parentView at the moment because it does not make much sense to force component authors to switch to an alternative now only to switch to scoped helpers based solution when that lands (hopefully in an early 2.x release).

tldr; what @cowboyd said 😺

@rwjblue
Copy link
Member

rwjblue commented Jul 21, 2015

@tpitale - The actual code changes to fix the behavior has changed (done a different way now), but all the tests added in emberjs/ember.js#11266 are still included and passing on beta and canary of Ember.

@tpitale
Copy link
Author

tpitale commented Jul 21, 2015

Thanks @rwjblue. I'll try beta 3.

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.

4 participants