-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat-fe: 평가 필터 팝오버 컴포넌트 구현 #804
Conversation
1728972223.636189 |
1728972224.071979 |
1728972224.040259 |
1728972234.523699 |
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.
러기 고생하셨어요! 복잡한 Input 요소를 포함한 컴포넌트를 멋지게 잘 만들어주셔서 감사합니다. 일부 브라우저에서 Slider 드래그 요소가 일그러지는 이슈가 있어서 RC 코멘트를 하나 드렸고, UI 아이디어 차원의 코멘트도 하나 남겨드렸으니 체크를 부탁드릴게요. 👍
appearance: none; | ||
|
||
&::-moz-range-thumb { | ||
width: 1.6rem; |
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.
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.
와 ㅋㅋ 환경별로 코드가 달라진다는게 진짜 이상하네용..
<Slider | ||
{...sliderProps} | ||
onRangeChange={handleRangeChange} | ||
/> |
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.
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.
아이고 이부분은 그냥 빠뜨린것 같습니다! 수정하도록 할게요
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.
popover 리팩토링한 거 너무 좋아요~ 간단한 제안 남겼으니 확인해주세요!
if (!isDisabled) { | ||
const newMinValue = Math.min(Number(e.target.value), Number(maxValue - step)); | ||
setMinValue(newMinValue); | ||
onRangeChange(newMinValue, maxValue); |
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.
새로운 값이 이전 값과 다른 경우에만 핸들러를 실행시켜서 최적화시켜줄 수도 있을 것 같아요~
onRangeChange(newMinValue, maxValue); | |
if (newMinValue !== minValue) onRangeChange(newMinValue, maxValue); |
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.
포매팅을 해주는 input에서는 이게 필수군요.. 한 수 배웠슴당 👍
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.
러기 고생하셨어요! 구현 및 개선 과정에서 여러 실험들을 진행해주시고, 그 과정에서 고민하신 지점과 대응책들을 세세히 공유해주셔서 정말 감사합니다. 기능, 구성, 디자인 측면에서 더 피드백 드릴 내용이 없어 Approve 드리겠습니다. 🙏
Original issue description
목적
작업 세부사항
참고 사항
RATING_FILTER_POPOVER
closes #803
다음과 같은 부분을 봐주시면 좋을 것 같아요!
PopoverProvider를 Popover 컴포넌트에서만 사용하도록 구현하기 위해서 Popove 내부에 Provider를 사용하신 걸 보실 수 있습니다. 사용자 입장에서 isOpen state를 자유롭게 사용하면서도 Provider를 사용하지 않으면서도 내부 children에는 Prop을 주입하지 않아도 되는 형태를 생각해봤어요!