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

Implement Slider component #871

Merged
merged 40 commits into from
Nov 7, 2022
Merged

Conversation

Dogdriip
Copy link
Contributor

@Dogdriip Dogdriip commented Jul 11, 2022

Summary

  • Form control 중 하나인 Slider 컴포넌트를 구현합니다.

Details

Closes #115

Preview

  • Chromatic에서 StoryBook 미리보기 및 직접 테스트가 가능합니다. PR 코멘트를 참고해 주시면 감사하겠습니다.
  • Story마다 Slider를 조절하면서 하단의 Actions 탭에서 onValueChange 핸들러 호출 상황을 볼 수 있습니다.

  • Uncontrolled: 기본 상태입니다. 컴포넌트의 기본 width는 36px이지만, 테스트의 편의를 위해 이하 Story에서는 285px로 보여줍니다.
Kapture.2022-09-21.at.14.21.50.mp4
  • Disabled: Slider inputdisabled 상태를 보여줍니다. 해당 상태에서는 핸들러가 trigger되지 않으며, 키보드 내비게이션이 되지 않습니다. cursornot-allowed, opacity는 Bezier의 DisabledOpacity를 가집니다.

image

  • With Guide: 가이드 눈금을 볼 수 있습니다. Sliderguide?: number[] prop이 제공되면 이에 따라 해당하는 value의 위치에 가이드 눈금을 표시합니다.
Kapture.2022-09-21.at.14.24.50.mp4
  • Multiple Thumbs: 두 개의 Thumb로 Slider를 Range input으로 사용할 수 있습니다. minStepsBetweenThumbs prop으로 Thumb 두 개가 하나의 value에 겹치지 않도록 할 수 있습니다. 키보드 내비게이션으로는 한 thumb가 다른 thumb를 넘어갈 수 없으며, 마우스 드래그로는 가능합니다. thumb가 넘어가더라도 (Slider의 왼쪽 thumb와 오른쪽 thumb가 달라지더라도) value는 오름차순을 유지합니다.
Kapture.2022-09-21.at.14.26.10.mp4

Future works

  • 드래그 중에는 Thumb에 현재 값을 나타내는 Tooltip이 떠야 합니다.
  • image - 단순히 Thumb element를 Tooltip으로 감싸면 Radix primitive가 이를 thumb로 인식하지 못하는 문제가 있어 일단 보류합니다. - Thumb의 정중앙을 따라다니는 가상의 element를 만들어서, Tooltip이 이를 따라다니도록 구현할 수 있어 보입니다.
  • 현재값에 따른 Thumb의 위치와 guide prop에 따른 Guide 눈금의 위치에 대한 단위 테스트를 작성해야 합니다.
    • 특히 가이드 눈금의 경우, 위치를 계산하는 로직이 다소 복잡하게 작성되었다고 생각합니다(Slider.styled.tscalculateGuideLeftPosition 참고). 추후에 위치 계산 로직을 더 간소화/추상화할 수 있다면 그렇게 하고 단위 테스트를 작성하면 좋겠습니다.

Browser Compatibility

OS / Engine 호환성을 반드시 확인해주세요.

Windows

  • Chrome - Blink
  • Edge - Blink
  • Firefox - Gecko (Option)

macOS

  • Chrome - Blink
  • Edge - Blink
  • Safari - WebKit
  • Firefox - Gecko (Option)

References

@Dogdriip Dogdriip added the enhancement Issues or PR related to making existing features better label Jul 11, 2022
@Dogdriip Dogdriip self-assigned this Jul 11, 2022
@changeset-bot
Copy link

changeset-bot bot commented Jul 11, 2022

🦋 Changeset detected

Latest commit: 89c62cf

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

codecov bot commented Jul 11, 2022

Codecov Report

Base: 71.00% // Head: 71.08% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (89c62cf) compared to base (82c8d17).
Patch coverage: 74.57% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           next-v1     #871      +/-   ##
===========================================
+ Coverage    71.00%   71.08%   +0.07%     
===========================================
  Files          204      206       +2     
  Lines         2894     2953      +59     
  Branches       807      818      +11     
===========================================
+ Hits          2055     2099      +44     
- Misses         721      736      +15     
  Partials       118      118              
Impacted Files Coverage Δ
...ezier-react/src/components/Forms/Slider/Slider.tsx 66.66% <66.66%> (ø)
...react/src/components/Forms/Slider/Slider.styled.ts 80.00% <80.00%> (ø)

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.

package.json Outdated Show resolved Hide resolved
@sungik-choi sungik-choi changed the title Implement Slider component (WIP) [WIP] Implement Slider component Jul 26, 2022
@guswnsxodlf
Copy link
Contributor

https://github.com/channel-io/ch-desk-web/pull/10917
여기서 onDragStart, onDragEnd Props가 추가되었습니다..!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2022

Chromatic Report

🚀 Congratulations! Your build was successful!

@Dogdriip Dogdriip changed the title [WIP] Implement Slider component Implement Slider component Sep 18, 2022
@Dogdriip Dogdriip marked this pull request as ready for review September 18, 2022 14:12
@inhibitor1217 inhibitor1217 self-requested a review September 20, 2022 08:28
@sungik-choi sungik-choi self-requested a review September 20, 2022 10:39
Copy link
Contributor

@inhibitor1217 inhibitor1217 left a comment

Choose a reason for hiding this comment

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

잘만드셨네용 😁

Comment on lines 33 to 36
export default interface SliderProps extends
BezierComponentProps,
SliderPrimitiveProps,
SliderOptions {}
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: hasError, readOnly prop 지원되면 좋을 것 같습니다. (FormComponentProps)

Copy link
Contributor

@guswnsxodlf guswnsxodlf left a comment

Choose a reason for hiding this comment

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

테스트 실패만 봐주시면 될 듯 합니다!

Copy link
Contributor

@sungik-choi sungik-choi left a comment

Choose a reason for hiding this comment

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

👍

Forms/Inputs/ 하위보단 Forms/ 하위가 적절할 거 같아요. 정의가 애매모호하긴한데, 지금은 Inputs 하위엔 텍스트 입력기와 스타일을 공유하는(Select) 컴포넌트만 위치해뒀습니다.

import { styled, css } from 'Foundation'
import { toLength } from 'Utils/styleUtils'
import DisabledOpacity from 'Constants/DisabledOpacity'
import { focusedInputWrapperStyle } from 'Components/Forms/Inputs/mixins'
Copy link
Contributor

Choose a reason for hiding this comment

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

만약 포커스 디자인이 TextField, TextArea, Select 외에도 범용적으로 사용된다면 Forms/ 디렉토리 하위로 이동해야할 거 같네요 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mixins 안에 focusedInputWrapperStyle, erroredInputWrapperStyle 이 두 개는 input 외에도 다른 컴포넌트에 사용할 수 있을 것 같아서, 후속 PR에서 input에 사용되는 믹스인과 그렇지 않은 것을 분리하고 적용해야 할 것 같습니다.

@Dogdriip Dogdriip merged commit 29c4a62 into channel-io:next-v1 Nov 7, 2022
@Dogdriip Dogdriip deleted the feat/slider branch November 7, 2022 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues or PR related to making existing features better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slider 컴포넌트 추가
5 participants