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

Axis block params #31

Merged
merged 1 commit into from Apr 14, 2015
Merged

Axis block params #31

merged 1 commit into from Apr 14, 2015

Conversation

ming-codes
Copy link
Contributor

Hi Ben,

This PR fixes #4.

Ming

@benlesh
Copy link
Contributor

benlesh commented Apr 14, 2015

This looks great. The only problem I see is we need a way to gracefully deprecate usage that isn't using block params. Otherwise this releases and "boom" all the axis in my projects break. ;)

I'm not entirely sure how to do this. Perhaps @stefanpenner or @jayphelps have thoughts?

One idea would be to hold on this PR (keeping it in another branch) for one release, and add a deprecation message in a release, then update it on the next release? It seems like there should be a way to default a block param. I just can't find it.

@stefanpenner
Copy link

i beleive one could detect the lookup of tick as a property and not a block param / keyword

@ming-codes
Copy link
Contributor Author

I just found that simply passing the component a template does not receive the block arguments. Component blocks are compiled differently than regular templates.

@benlesh
Copy link
Contributor

benlesh commented Apr 14, 2015

@stefanpenner ... so are you saying one could create a property tick that griped?

tick: Ember.computed(function() {
   Ember.Logger.error('OMG, you forgot "as |tick|" on your nf-x-axis component!');
}),

That would be helpful, I guess. Not necessarily graceful. Perhaps a combination of that and the deprecation message?

@benlesh
Copy link
Contributor

benlesh commented Apr 14, 2015

I just found that simply passing the component a template does not receive the block arguments.

What do you mean?

@ming-codes
Copy link
Contributor Author

@Blesh Standalone template (nf-x-axis-tick.hbs) does not receive the block arguments because it has no context of the outer scope. Originally, I wanted axis use a default template is a block argument is not set. My latest commit fixes that problem.

Also, I think I slightly misunderstood what you mean by "gracefully deprecate usage". The default template was intended for that purpose, but I guess you meant the originally injected tick name.

@benlesh
Copy link
Contributor

benlesh commented Apr 14, 2015

@Lightblade I guess what I'm saying is that the following doesn't render anything:

{{#nf-x-axis}}
  <text>{{tick.value}}</text>
{{/nf-x-axis}}

If that's the case, the moment a developer upgrades, all of their graph axes will be broken.

To be clear, I want to move to block params, but I want to be able to do so incrementally, and I'm having a hard time wrapping my head around the best course of action.

@ming-codes
Copy link
Contributor Author

I agree. But this actually renders nothing in the current version:

{{#nf-x-axis as |tick|}}
  <text>{{tick.value}}</text>
{{/nf-x-axis}}

I'm thinking of having the none block-param version prints out a deprecation message. But find some way to make both syntax work in the current version.

@jayphelps
Copy link
Member

How bout this? Graceful deprecation.

http://emberjs.jsbin.com/wabile/3/edit

@ming-codes
Copy link
Contributor Author

@jayphelps 👍

Looks good. I'll find some way to work that into the PR.

@benlesh
Copy link
Contributor

benlesh commented Apr 14, 2015

Very awesome, guys! I'm excited about this PR.

@ming-codes
Copy link
Contributor Author

Done. @jayphelps @Blesh Please review.

@jayphelps
Copy link
Member

@Lightblade can you squash your commits please? 👯

Get around the problem of standalone template not receiving blockArguments

allow axis ticks to work w/ or w/o block param

added deprecation messages for tick non block form
@ming-codes
Copy link
Contributor Author

Done.

@benlesh
Copy link
Contributor

benlesh commented Apr 14, 2015

Awesome! Tested this out and it looks great! Thanks, Ming.

benlesh added a commit that referenced this pull request Apr 14, 2015
@benlesh benlesh merged commit 07080d9 into Netflix:master Apr 14, 2015
@jayphelps
Copy link
Member

Thanks @Lightblade!

@ming-codes
Copy link
Contributor Author

You're welcome!

@ming-codes ming-codes deleted the axis-block-params branch April 15, 2015 01:33
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.

Use block params
4 participants