Skip to content

Extract File menu into a component#128

Merged
rwjblue merged 4 commits intoember-cli:masterfrom
Gaurav0:file_menu
Aug 4, 2015
Merged

Extract File menu into a component#128
rwjblue merged 4 commits intoember-cli:masterfrom
Gaurav0:file_menu

Conversation

@Gaurav0
Copy link
Copy Markdown
Contributor

@Gaurav0 Gaurav0 commented Aug 4, 2015

Relevant to issue #127

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Aug 4, 2015

Thank you, I definitely like this! A few tweaks:

  • Can you swap the internalAction="someActionFromControlleR" and this.sendAction usage for the newer style closure actions?
  • Can you split up the test a bit (I'd like to see a separate test for each action)?

@Gaurav0
Copy link
Copy Markdown
Contributor Author

Gaurav0 commented Aug 4, 2015

@rwjblue Updated. Some of the actions could not be made into closure actions because they bubble to the route.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you tweak these to save to this? Something like:

this.on('addFile', (type) => { this.addFileCalled = true; this.fileType = type; });

Then you can assert in the tests below like:

assert.ok(this.addFileCalled, 'addFile was called');
assert.equal(this.fileType, 'template', 'addFile was called with type parameter');

Using the above, will help us ensure that there is no leakage between tests. Say we add a test in the future that also triggers an action, the boolean xyzCalled may have already been set causing the tests to be not quite right.

@Gaurav0
Copy link
Copy Markdown
Contributor Author

Gaurav0 commented Aug 4, 2015

@rwjblue Updated again as per request.

@Gaurav0 Gaurav0 removed the working label Aug 4, 2015
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is totally fine as is, but removing these would result in the same test (since this.addFileCalled would be undefined and not truthy the assertions would still pass).

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Aug 4, 2015

👍 - Sorry to be such an annoying reviewer, but thanks for working so hard on this!

rwjblue added a commit that referenced this pull request Aug 4, 2015
Extract File menu into a component
@rwjblue rwjblue merged commit 049c6ba into ember-cli:master Aug 4, 2015
@Gaurav0 Gaurav0 deleted the file_menu branch March 21, 2016 13:49
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.

3 participants