Enhance the `Slider` Component #1070

guswnsxodlf · 2022-12-07T11:51:13Z

Self Checklist

I wrote a PR title in English.
I added an appropriate label to the PR.
I wrote a commit message in English.
I wrote a commit message according to the Conventional Commits specification.
I added the appropriate changeset for the changes.
[Component] I wrote a unit test about the implementation.
[Component] I wrote a storybook document about the implementation.
[Component] I tested the implementation in various browsers.
- Windows: Chrome, Edge, (Optional) Firefox
- macOS: Chrome, Edge, Safari, (Optional) Firefox
[New Component] I added my username to the correct directory in the CODEOWNERS file.

Summary

Enhance/fix the Slider component

Details

Enhancement

It now observes the value prop and change the currentValue state.

Fix

Render the thumbs through currentValue state instead of defaultValue prop.
Fix: Always rerender when the currentValue is changed

Screen.Recording.2022-12-07.at.11.41.10.mov

Breaking change or not (Yes/No)

No

References

None

changeset-bot · 2022-12-07T11:51:16Z

🦋 Changeset detected

Latest commit: 9922c64

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@channel.io/bezier-react	Patch
bezier-figma-plugin	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2022-12-07T11:55:01Z

Codecov Report

Base: 71.72% // Head: 71.94% // Increases project coverage by +0.21% 🎉

Coverage data is based on head (9922c64) compared to base (e23c54a).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           next-v1    #1070      +/-   ##
===========================================
+ Coverage    71.72%   71.94%   +0.21%     
===========================================
  Files          217      217              
  Lines         3056     3062       +6     
  Branches       842      845       +3     
===========================================
+ Hits          2192     2203      +11     
+ Misses         740      733       -7     
- Partials       124      126       +2

Impacted Files	Coverage Δ
...ezier-react/src/components/Forms/Slider/Slider.tsx	`90.00% <80.00%> (+23.33%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2022-12-07T11:57:11Z

Chromatic Report

🚀 Congratulations! Your build was successful!

packages/bezier-react/src/components/Forms/Slider/Slider.test.tsx

Dogdriip · 2022-12-08T04:52:47Z

packages/bezier-react/src/components/Forms/Slider/Slider.tsx

          />
        )) }
-        { defaultValue.map((v) => (
+        { currentValue.map((v) => (


packages/bezier-react/src/components/Forms/Slider/Slider.tsx

guswnsxodlf · 2022-12-09T09:45:48Z

실제로 인터렉션 해야하는건 어떻게 테스트 해야할지..? 예제가 혹시 있나요?

sungik-choi · 2022-12-12T08:34:11Z

실제로 인터렉션 해야하는건 어떻게 테스트 해야할지..? 예제가 혹시 있나요?

userEvent 를 사용한 테스트를 말씀하시는 게 맞을지 모르겠네요.

Dogdriip · 2022-12-14T01:54:10Z

실제로 인터렉션 해야하는건 어떻게 테스트 해야할지..? 예제가 혹시 있나요?

https://github.com/channel-io/bezier-react/blob/next-v1/packages/bezier-react/src/components/Forms/Switch/Switch.test.tsx#L163-L182

Switch 테스트에서 클릭 인터랙션을 테스트하는 부분이 있고, 나아가 https://testing-library.com/docs/user-event/pointer 를 참고해보면 클릭 후 드래그 등의 인터랙션 테스트도 작성 가능해 보입니다.

pointer([
  // touch the screen at element1
  {keys: '[TouchA>]', target: element1},
  // move the touch pointer to element2
  {pointerName: 'TouchA', target: element2},
  // release the touch pointer at the last position (element2)
  {keys: '[/TouchA]'},
])

guswnsxodlf · 2022-12-19T12:54:02Z

@sungik-choi
테스트 코드 작성에 앞서, codecov/patch에서 말하는 L50 (handleValueChange)가 테스트 되어야 할 필요가 있는지 다시 한번 생각해봐야 할 것 같네요, 이 함수가 실행이 잘 되었는지에 대한 테스트는 결국 radix-ui 코드를 테스트하는 것인데, 그건 우리의 책임이 아닙니다.

sungik-choi · 2022-12-20T04:33:29Z

@sungik-choi 테스트 코드 작성에 앞서, codecov/patch에서 말하는 L50 (handleValueChange)가 테스트 되어야 할 필요가 있는지 다시 한번 생각해봐야 할 것 같네요, 이 함수가 실행이 잘 되었는지에 대한 테스트는 결국 radix-ui 코드를 테스트하는 것인데, 그건 우리의 책임이 아닙니다.

저는 오히려 radix-ui 코드를 테스트하는 게 필요하다고 생각하는 편입니다. 외부 라이브러리 버전이 업데이트되더라도 저희 라이브러리의 일관된 동작을 쉽게 보장할 수 있으려면 현재 버전에서 컴포넌트에 기대하는 동작들에 대한 다양한 테스트 케이스가 필요하다고 생각해요. 더해서 테스트를 잘 작성해두면 추후 내부 구현에 radix-ui가 아닌 다른 라이브러리를 사용한다고 해도 동일하게 동작함을 어느정도 보장할 수도 있다고 생각합니다.

guswnsxodlf · 2022-12-20T18:48:09Z

Radix-ui 의 Slider에서 내부적으로 element.hasPointerCapture() API를 사용하고 있는 것 같은데, 이는 jest가 사용하고 있는 JSDOM에서는 구현되지 않은 api이기 때문에 해당 method가 없다고 나오는군요.. (관련 이슈)

해당 method를 mocking해서 사용하던지 해야할 것 같은데, JSDOM이 완벽히 브라우저와 동일하게 작동한다고 보장하긴 어려울 것 같네요. (비슷한 예를 설명한 아티클)

유저 인터렉션 관련한 부분은 브라우저 시뮬레이터로 테스트하는 것이 리소스는 더 먹겠지만 정확한 방법이라는 생각도 듭니다.

sungik-choi · 2022-12-21T05:05:54Z

Radix-ui 의 Slider에서 내부적으로 element.hasPointerCapture() API를 사용하고 있는 것 같은데, 이는 jest가 사용하고 있는 JSDOM에서는 구현되지 않은 api이기 때문에 해당 method가 없다고 나오는군요..

해당 method를 mocking해서 사용하던지 해야할 것 같은데, JSDOM이 완벽히 브라우저와 동일하게 작동한다고 보장하긴 어려울 것 같네요. (비슷한 예를 설명한 아티클)

유저 인터렉션 관련한 부분은 브라우저 시뮬레이터로 테스트하는 것이 리소스는 더 먹겠지만 정확한 방법이라는 생각도 듭니다.

👍 스토리북에 인터랙션 테스트 기능이 있는데 관련해서 알아보면 좋을 거 같네요. 우선은 mocking해서 가능하다면 그 쪽으로 해결해주시면 좋을 거 같습니다!

sungik-choi · 2022-12-26T08:22:07Z

packages/bezier-react/jest.setup.ts

+/**
+ * Radix-ui uses the APIs below, but the DOM in jest (JSDOM) hasn't implemented them.
+ * @see https://github.com/radix-ui/primitives/issues/1822
+ */


guswnsxodlf added 4 commits December 7, 2022 11:34

feat: change currentValue through value prop change

97188a8

fix: render thumbs through currentValue props

2756764

docs: add value prop on storybook

ff6fd8c

chore: add changeset

e605245

guswnsxodlf added enhancement Issues or PR related to making existing features better component labels Dec 7, 2022

guswnsxodlf requested review from Dogdriip and sungik-choi as code owners December 7, 2022 11:51

guswnsxodlf self-assigned this Dec 7, 2022

Dogdriip approved these changes Dec 8, 2022

View reviewed changes

guswnsxodlf force-pushed the enhance/slider branch from e25a54e to 12d2107 Compare December 8, 2022 12:00

test: add a test case

2e7a863

guswnsxodlf force-pushed the enhance/slider branch from 12d2107 to 2e7a863 Compare December 8, 2022 12:01

sungik-choi reviewed Dec 9, 2022

View reviewed changes

packages/bezier-react/src/components/Forms/Slider/Slider.tsx Outdated Show resolved Hide resolved

refactor: name useEffect function

6a95b22

guswnsxodlf requested a review from sungik-choi December 9, 2022 09:34

refactor: set default value on the function props

7ec97eb

guswnsxodlf force-pushed the enhance/slider branch from a522c7c to 7ec97eb Compare December 9, 2022 09:40

sungik-choi approved these changes Dec 13, 2022

View reviewed changes

test(Slider): divide a test case to 2 cases

2dc8cbd

guswnsxodlf added the fix PR related to bug fix label Dec 22, 2022

guswnsxodlf requested a review from sungik-choi December 22, 2022 12:09

guswnsxodlf added 3 commits December 22, 2022 13:15

refactor: forwardedRef type

ebc9a34

fix: always rerender when the current value is changed

bda0af9

test: add user interaction test case

9922c64

guswnsxodlf force-pushed the enhance/slider branch from b7596dd to 9922c64 Compare December 22, 2022 15:08

sungik-choi approved these changes Dec 26, 2022

View reviewed changes

guswnsxodlf merged commit 78d217e into channel-io:next-v1 Dec 26, 2022

guswnsxodlf deleted the enhance/slider branch December 26, 2022 09:00

github-actions bot mentioned this pull request Dec 26, 2022

ci(changesets): version packages (next-v1) #1087

Merged

github-actions bot mentioned this pull request May 30, 2023

ci(changesets): version packages #1369

Merged

Enhance the Slider Component #1070

Enhance the Slider Component #1070

Uh oh!

Conversation

guswnsxodlf commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Self Checklist

Summary

Details

Breaking change or not (Yes/No)

References

Uh oh!

changeset-bot bot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

codecov bot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Chromatic Report

Uh oh!

Uh oh!

Dogdriip Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guswnsxodlf commented Dec 9, 2022

Uh oh!

sungik-choi commented Dec 12, 2022

Uh oh!

Dogdriip commented Dec 14, 2022

Uh oh!

guswnsxodlf commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sungik-choi commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guswnsxodlf commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sungik-choi commented Dec 21, 2022

Uh oh!

sungik-choi Dec 26, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Enhance the `Slider` Component #1070

Enhance the `Slider` Component #1070

guswnsxodlf commented Dec 7, 2022 •

edited

Loading

changeset-bot bot commented Dec 7, 2022 •

edited

Loading

codecov bot commented Dec 7, 2022 •

edited

Loading

github-actions bot commented Dec 7, 2022 •

edited

Loading

guswnsxodlf commented Dec 19, 2022 •

edited

Loading

sungik-choi commented Dec 20, 2022 •

edited

Loading

guswnsxodlf commented Dec 20, 2022 •

edited

Loading