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.

Sign up for GitHub

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

Jump to bottom

Stef's code review #44

Closed
mike-north opened this issue Apr 3, 2015 · 5 comments
Closed

Stef's code review #44

mike-north opened this issue Apr 3, 2015 · 5 comments
Labels
discussion in progress

Comments

@mike-north
Copy link
Collaborator

@stefanpenner Please review this before we start including it in a large number of Yahoo ember apps :)
@poteto
Copy link
Collaborator

poteto commented Apr 4, 2015

@truenorth Cool, let me know how I can help! :)

@stefanpenner
Copy link

this project looks great, really nice!

This pass is mostly superficial, I will take some time tomorrow to use this in an app, and give better feedback.

I'm leaving some my comments in the form of todos, so that myself or someone else interested can make the appropriate updates :)

@poteto
Copy link
Collaborator

poteto commented Apr 8, 2015

Thanks for the review @stefanpenner!

poteto added a commit that referenced this issue Apr 8, 2015
@poteto
Addresses points from #44
fbb503f 
- props that should be readOnly are now actually readOnly
- added tests for readOnly props in flash model
- stop using var in tests
- use Ember.EnumerableUtils instead of array prototype methods
- added tests for readOnly prop on service
- reorganised variable declarations in service test
- added tests for readOnly props on component
- fixed missing super
- fixed setting state on prototype
- fixed indentation issue in service
poteto added a commit that referenced this issue Apr 8, 2015
@poteto
Addresses points from #44
6f38ed3 
- props that should be readOnly are now actually readOnly
- added tests for readOnly props in flash model
- stop using var in tests
- use Ember.EnumerableUtils instead of array prototype methods
- added tests for readOnly prop on service
- reorganised variable declarations in service test
- added tests for readOnly props on component
- fixed missing super
- fixed setting state on prototype
- fixed indentation issue in service
poteto added a commit that referenced this issue Apr 9, 2015
@poteto
Addresses points from #44
c56a0eb 
- props that should be readOnly are now actually readOnly
- added tests for readOnly props in flash model
- stop using var in tests
- use Ember.EnumerableUtils instead of array prototype methods
- added tests for readOnly prop on service
- reorganised variable declarations in service test
- added tests for readOnly props on component
- fixed missing super
- fixed setting state on prototype
- fixed indentation issue in service
- removed redundant private method in flash model
- updated tests
- addded Evented mixin to flash object
- added lookup helpers in testing
- made default sticky in testing true
@poteto poteto mentioned this issue Apr 9, 2015
Merged
@mike-north
Copy link
Collaborator Author

Thanks for making all of these improvements @poteto. We really appreciate it!

@poteto poteto closed this as completed Jun 28, 2015
@stefanpenner
Copy link

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discussion in progress
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@stefanpenner @mike-north @poteto