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

update confusing usage example in readme #38

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

jelhan
Copy link
Contributor

@jelhan jelhan commented Aug 15, 2018

Example assumed that component yields a hash before but a component after changing syntax. Since angle bracket invocation does not affect yielded value it should be the same before and after.

It's the same in RFC. Not quite sure if the RFC should be changed also even so it's already merged.

README.md Outdated
{{#each this.availableCountries as |country|}}
{{#s.option value=country}}{{country.name}}{{/s.option}}
{{#option value=country}}{{country.name}}{{/s.option}}
Copy link

@knownasilya knownasilya Aug 15, 2018

Choose a reason for hiding this comment

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

You can't call a non-angle bracket component without a dash, that is why the API is different. To make it similar you'd use {{#component option value=country}}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. Missed that one. So maybe it should be done the other way round? Assuming it returns a hash and updating the angle bracket syntax accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is it attended to show this difference? At least without an explanation that was very confusing to me.

Choose a reason for hiding this comment

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

The other way around makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR accordingly.

@jelhan jelhan force-pushed the fix-confusing-usage-example branch from 72f5227 to 86ec2f0 Compare August 15, 2018 17:13
@jelhan
Copy link
Contributor Author

jelhan commented Aug 15, 2018

Please note, that <s.option> violates template linting: ember-template-lint/ember-template-lint#473

README.md Outdated Show resolved Hide resolved
Example assumed that component yields a hash before but a component after
changing syntax. Since angle bracket invocation does not affect yielded
value it should be the same before and after.

[ci skip]
@jelhan jelhan force-pushed the fix-confusing-usage-example branch from 86ec2f0 to 5916976 Compare August 30, 2018 16:02
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thank you!

@rwjblue rwjblue merged commit f8eef14 into ember-polyfills:master Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants