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.

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

Merged
merged 8 commits into from
Aug 9, 2021
Merged

Remove responsibility for managing state from [ButtonGroup] #56

merged 8 commits into from
Aug 9, 2021

Conversation

yujonglee
Copy link
Contributor

@yujonglee yujonglee commented Aug 8, 2021

Description

  1. given and context are awesome libraries to write readable test codes. Although I use only context here, they usually work together. So I add both to commit.
  2. [ButtonGroup] doesn't need to be responsible for managing selected option. It makes testing hard(in general), and reduces reusability and flexibility of component.

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Run yarn test or yarn test -u if you need to update snapshot.
  • Run yarn lint
  • I am willing to follow-up on review comments in a timely manner.

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
Copy link

codecov bot commented Aug 8, 2021

Codecov Report

Merging #56 (66da1a7) into master (30d9491) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            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              

@hyochan hyochan added the 🍗 enhancement New feature or request label Aug 8, 2021
@hyochan
Copy link
Owner

hyochan commented Aug 8, 2021

@yujong-lee Yeah jest-plugin-context looks awesome. But I am afraid that this isn't been maintained for 3 years 🤔

@hyochan
Copy link
Owner

hyochan commented Aug 8, 2021

Also we need to add @types/jest-plugin-context for typescript

@hyochan hyochan added the 🧪 test Issue or pr related to testing label Aug 8, 2021
'selected' feels like it's boolean. Rename to be clear.
@yujonglee
Copy link
Contributor Author

yujonglee commented Aug 9, 2021

Ok. I rename variable and install @types/jest-plugin-context.

There's few things I want to share.

  1. context is just aliases of describe.
    See source code. So In this case, I think the fact that this library is not maintained for 3 years doesn't really matters.

  2. But, given2 is more complex library, and it's not maintaining these days too.
    So I find two alternative library. givens and bdd-lazy-var.

givens : similar interface with given2. Which is familiar to me and other RSpec-background-developers. Auther said it has advantages over given2and bdd-lazy-var. But I haven't used/heard of it so it needs some testing. It is also maintained by single author.

bdd-lazy-var : different interface with given2. Has more feature than given2, but they are not crucial.(in my opinion.) Good documentation. Many contributors. Updated(except bot) less than 1 year. It has built-in context aliases.

I'm not confident to choose one now. (jest-plugin-context + given2 or givens / bdd-lazy-var)
So what I suggest is add this talk to discussion. remove given2 from this PR, and choose among these options when it is needed. (It will be needed soon).

@yujonglee yujonglee mentioned this pull request Aug 9, 2021
@yujonglee yujonglee requested a review from hyochan August 9, 2021 03:30
render(
<ThemeProvider initialThemeType={themeType}>
<ButtonGroup
initialIndex={0}
selected={index}
Copy link
Owner

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

Copy link
Contributor Author

@yujonglee yujonglee Aug 9, 2021

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.

@hyochan
Copy link
Owner

hyochan commented Aug 9, 2021

Ok. I rename variable and install @types/jest-plugin-context.

There's few thing I want to share.

  1. context is just aliases of describe.
    See source code. So In this case, I think the fact that this library is not maintained for 3 years doesn't really matters.
  2. But, given2 is more complex library, and it's not maintaining these days too.
    So I find two alternative library. givens and bdd-lazy-var.

givens : similar interface with given2. Which is familiar to me and other RSpec-background-developers. Auther said it has advantages over given2and bdd-lazy-var. But I haven't used/heard of it so it needs some testing. It is also maintained by single author.

bdd-lazy-var : different interface with given2. Has more feature than given2, but they are not crucial.(in my opinion.) Good documentation. Many contributors. Updated(except bot) less than 1 year. It has built-in context aliases.

I'm not confident to choose one now. (jest-plugin-context + given2 or givens / bdd-lazy-var)
So what I suggest is add this talk to discussion. remove given2 from this PR, and choose among these options when it is needed. (It will be needed soon).

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.
@yujonglee
Copy link
Contributor Author

No problem!

Copy link
Owner

@hyochan hyochan left a comment

Choose a reason for hiding this comment

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

LGTM!

@hyochan hyochan merged commit ab9bf3f into hyochan:master Aug 9, 2021
@yujonglee yujonglee deleted the ButtonGroup/state branch August 9, 2021 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement New feature or request 🧪 test Issue or pr related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants