-
Notifications
You must be signed in to change notification settings - Fork 126
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
Array helpers #185
Array helpers #185
Conversation
taras
commented
Jul 19, 2016
- Added map helper
- Added reduce helper
- Added filter helper
- Updated README
@@ -79,8 +79,11 @@ For action helpers, this will mean better currying semantics: | |||
+ [`underscore`](#underscore) | |||
+ [`w`](#w) | |||
* [Array](#array-helpers) | |||
+ [`array`](#array) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You moved array to the top, but didn't move the section in the document itself. Either move this one back, or move the section to the correct order. I'd prefer moving this entry back to it's original place, as I don't see the need why this needs to be moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move it back, but I think it should be at the top because it's one of the most primitive helpers in this list. People need to know that it's available. I'm sure they'll come across it eventually but why not put it at the top so it's right there.
In summary:
|
@martndemus I've done everything. |
|
||
export default Helper.extend({ | ||
compute([callback, array]) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@poteto sorry, I'm not sure what you mean. I checked for extra white space and I don't see it. I also looked at how filter-by
is implemented and I don't see a difference there either. Could you please tell me what you'd like me to change about the whitespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think she means delete the empty line at L13
Looks good, mostly minor formatting and all tests need precondition assertions |
@poteto I've made the changes that you suggested. |
@taras you still need to fix changing |
- Removed empty line from beginning of compute method in filter, filter-by, map and reduce - Replaced the remaining this.set with this.on - Renamed actions called callback to semantically better names
@martndemus sorry, I missed a few |
}); | ||
|
||
this.render(hbs` | ||
{{~#each (filter (action 'truthyFoo') array) as |item|~}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use double quotes in HBS templates.
If you can fix the double quotes issue and then squash all your commits into one, I think we're good to merge. |
@martndemus done |
@taras Squash your commits please! |
You can squash PRs in Github @martndemus. I'm outside right now so can't make a release |
Published |
Awesome. Thank you |