-
Notifications
You must be signed in to change notification settings - Fork 31
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
Remove responsibility for managing state from [ButtonGroup] #56
Conversation
ButtonGroup doesn't need to be responsible for managing selected option. It makes testing hard(in general), and reduces reusability and flexibility of component.
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
==========================================
+ Coverage 93.91% 93.93% +0.02%
==========================================
Files 21 21
Lines 542 544 +2
Branches 231 235 +4
==========================================
+ Hits 509 511 +2
Misses 33 33 |
@yujong-lee Yeah jest-plugin-context looks awesome. But I am afraid that this isn't been maintained for 3 years 🤔 |
Also we need to add |
'selected' feels like it's boolean. Rename to be clear.
Ok. I rename variable and install There's few things I want to share.
I'm not confident to choose one now. ( |
main/__tests__/ButtonGroup.test.tsx
Outdated
render( | ||
<ThemeProvider initialThemeType={themeType}> | ||
<ButtonGroup | ||
initialIndex={0} | ||
selected={index} |
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.
Looks like the props here is not changed yet selectedIndex
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.
Sorry for that.
This happens due to insufficient test that didn't caught my mistake.
I updated test codes to test default value of selectedIndex
, check test fails, and fix mistake to pass it.
It looks like a great discussion to start! Could you kindly open up a discussion? |
1. Reflect prop name change. 2. Add test for situation when selectedIndex is not given.
No problem! |
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.
LGTM!
Description
given
andcontext
are awesome libraries to write readable test codes. Although I use onlycontext
here, they usually work together. So I add both to commit.Related Issues
#52
Tests
I update test code first. Then modify [ButtonGroup] to fulfill tests.
So changes in test code represents what I did in [ButtonGroup].
Checklist
yarn test
oryarn test -u
if you need to update snapshot.yarn lint