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

Fix borderRadius-related bug in [ButtonGroup] #62

Merged
merged 3 commits into from
Aug 10, 2021
Merged

Fix borderRadius-related bug in [ButtonGroup] #62

merged 3 commits into from
Aug 10, 2021

Conversation

yujonglee
Copy link
Contributor

@yujonglee yujonglee commented Aug 9, 2021

Description

  1. See Top-Right and Botton-Right radius. It is not working properly when data size is 1.
    So I write tests, fix bug, and refactor code.
    (remove chained ternaries as mentioned in Remove some chained ternaries #45)
    이미지 2021  8  9  오후 10 50

  2. In the process of fixing bug, I made trivial changes.

  • Move CHILD_index testID to make testing easier.
  • Remove useless theme prop / default value
  • Remove testID prop
    I don't think user need testID prop. It is applied to Outermost View wrapper. But why user need to get that? They might write test code relate to each button. But they can to that with getByText or element_index testID. So it is no need to give them free to name it. Just name 'container' is fine for development use.
  • Rename testID ([userInput - CHILD] -> [container - element])
    I think container - element gives better abstraction.
  • Data prop shouldn't be optional. I remove default value and make it required.
  1. I use givens library. I will explain reasons in discussion soon.
    Although it supports Typescript, I didn't define types because I think It's bit much for test code.
import getGiven from 'givens';
interface myVars {
  var1: number; // the keys you intend to use
  var2: string; // and their types
}
const given = getGiven<myVars>();

Related Issues

#45

Tests

I add tests to ensure [ButtonGroup] calculates proper borderWidth and borderRadius.

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.

I fould borderRadius is not applied properly when data size is 1.
So I can't trust code that sets border width and radius.

I write test code about border, and rewrite original code.

There's some trivial changes too. I mentioned it in Pull Request.
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #62 (5a10afb) into master (63175fd) will increase coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   93.94%   94.22%   +0.27%     
==========================================
  Files          21       21              
  Lines         545      554       +9     
  Branches      236      235       -1     
==========================================
+ Hits          512      522      +10     
+ Misses         33       32       -1     

@yujonglee
Copy link
Contributor Author

yujonglee commented Aug 9, 2021

I should have split trival changes to different PR. 😅

At least props change like theme and testID shoud split to other PR to make changes clear. (And they need docs updates too)

Edit: I did it.

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.

You've rocked!! 👊

@hyochan hyochan added the 🍗 enhancement New feature or request label Aug 10, 2021
@hyochan hyochan merged commit f41047a into hyochan:master Aug 10, 2021
@yujonglee yujonglee deleted the yj/ButtonGroup/border branch August 10, 2021 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants