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

Use Ember.lookup as context for {{CONSTANT}}. Fixes #3098 #3218

Closed
wants to merge 1 commit into from

Conversation

xtian
Copy link
Contributor

@xtian xtian commented Aug 27, 2013

This fixes the discrepancy in context between {{CONSTANT}} and {{#each CONSTANT}}{{this}}{{/each}} as described in #3098.

Is this the right way to fix this? I'm not sure if this change has unintended consequences.

@wagenet
Copy link
Member

wagenet commented Sep 10, 2013

@xtian Can you update this to follow the guidelines here: http://emberjs.com/guides/contributing/adding-new-features/?

@stefanpenner
Copy link
Member

@xtian this looks good to me, do you mind following the adding-new-features guideline peter suggested?

Although this does seem like a bugfix, we will likely treat it as a new feature.

@xtian
Copy link
Contributor Author

xtian commented Sep 26, 2013

Whoops, I forgot about this. Yep, I'll do that now.

@stefanpenner
Copy link
Member

@xtian thanks dude.

@wagenet
Copy link
Member

wagenet commented Sep 27, 2013

@stefanpenner should this be flagged too? It's sort of an odd case.

@xtian
Copy link
Contributor Author

xtian commented Oct 22, 2013

@stefanpenner I think this is ready now

@wagenet
Copy link
Member

wagenet commented Nov 1, 2013

@xtian this doesn't merge cleanly. Can you rebase on master?

@xtian
Copy link
Contributor Author

xtian commented Nov 2, 2013

@wagenet Rebased

@twokul
Copy link
Member

twokul commented Nov 6, 2013

@wagenet ping

@wagenet
Copy link
Member

wagenet commented Nov 9, 2013

Merged fc4df51.

@wagenet wagenet closed this Nov 9, 2013
@xtian xtian deleted the constant-consistency branch November 9, 2013 19:30
@stefanpenner stefanpenner mentioned this pull request Jan 3, 2014
8 tasks
@ebryn
Copy link
Member

ebryn commented Jan 31, 2014

Thanks for your work on this PR, but this was no-go'd by the core team and will need to be reverted.

The behavior we want is for this to always be a local path lookup. We're deprecating the global lookup. Since this would be a breaking change, in the meantime, we'd like to default to local path lookup and then fallback to global path lookup with a deprecation warning.

@xtian Would you mind resubmitting a PR matching this behavior?

@xtian
Copy link
Contributor Author

xtian commented Jan 31, 2014

@ebryn Sure, I'll take a look at that. So I should just remove this feature flag entirely, right?

@ebryn
Copy link
Member

ebryn commented Jan 31, 2014

The behavior I described wouldn't need a feature flag, it would be considered a bugfix.

@tomdale
Copy link
Member

tomdale commented Feb 7, 2014

@xtian Do you think you will have time to work on a PR for this next week? If not, we can find someone to hand it off to so we can get this all shipped. Thanks!

@xtian
Copy link
Contributor Author

xtian commented Feb 7, 2014

@tomdale Yep, I'm looking at it right now. Sorry, this week ended up being busier for me than I anticipated.

mixonic pushed a commit to mixonic/ember.js that referenced this pull request Jun 23, 2014
rwjblue added a commit that referenced this pull request Jul 25, 2014
Revert #3218, replace #4358. Deprecate global access from templates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants