Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

Break up the render method logic into functions #1148

Merged
merged 2 commits into from
Jul 6, 2017

Conversation

el-mapache
Copy link
Contributor

@el-mapache el-mapache commented Jun 30, 2017

  • Adds two components to represent a link and button.
  • Removes nested if/else statements in favor of smaller named functions

@el-mapache el-mapache self-assigned this Jun 30, 2017
@el-mapache el-mapache force-pushed the ab-refactor-action branch from 4c60604 to 5cd9f0d Compare June 30, 2017 15:49
@msecret
Copy link
Contributor

msecret commented Jun 30, 2017

Great idea @el-mapache! Looking much cleaner

@el-mapache el-mapache force-pushed the ab-refactor-action branch 2 times, most recently from 5b50290 to e0a8a4c Compare July 3, 2017 11:35
@el-mapache
Copy link
Contributor Author

Thanks! I'm hoping to hit up a couple more spots in the code this week as well.

@el-mapache el-mapache force-pushed the ab-refactor-action branch from e0a8a4c to d79e326 Compare July 3, 2017 12:19
@el-mapache
Copy link
Contributor Author

@jcscottiii @rememberlenny I commented out one of the specs that tests that the click handler is called from the child component of . I can't get it to pass, although manual testing verifies that it works. If ya'll have any ideas for why it might be failing, they would be much appreciated.

expect(clickHandlerSpy).toHaveBeenCalledOnce();
});
xit('triggers clickHandler', () => {
action.find(Button).simulate('click');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't think of why this wouldn't work besides that it's a simulated click. But even then, there's an exact example in the enzyme docs with sinon, pretty much just like this.

I wouldn't say this is the most important thing to test as I don't think there are any logic branches in the functionality that calls the click handler

@jcscottiii
Copy link
Contributor

jcscottiii commented Jul 3, 2017

I switched to using mount instead of shallow then it worked. no clue why though.

Was looking at this when I decided to make the change: https://github.com/airbnb/enzyme#full-dom-rendering

@el-mapache
Copy link
Contributor Author

mount renders all the child components as well, I was hoping to avoid it, but maybe we have to use it in this case! Thanks for taking a look.

@el-mapache
Copy link
Contributor Author

@jcscottiii is this good to be merged?

Copy link
Contributor

@jcscottiii jcscottiii left a comment

Choose a reason for hiding this comment

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

This looks good! Just need to rebase / fix conflicts

* Adds two components to represent a link and button.
* Removes nested if/else statements in favor of smalled named functions

Rename folder, trying to get all the specs passing
@el-mapache el-mapache force-pushed the ab-refactor-action branch from 5c53988 to 87a89b0 Compare July 6, 2017 18:21
@el-mapache
Copy link
Contributor Author

@jcscottiii All updated!

jcscottiii
jcscottiii previously approved these changes Jul 6, 2017
Copy link
Contributor

@jcscottiii jcscottiii left a comment

Choose a reason for hiding this comment

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

one last thing. there's that skipped test.

@jcscottiii
Copy link
Contributor

one more thing, can we use mount so we can unskip this test? https://github.com/18F/cg-dashboard/pull/1148/files#diff-2a387c4f08d0883724c81e45f5f494a4R83

The shallowly rendered component wasn't registering click events
@jcscottiii jcscottiii merged commit 2e272ce into master Jul 6, 2017
@jcscottiii jcscottiii deleted the ab-refactor-action branch July 6, 2017 21:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants