-
Notifications
You must be signed in to change notification settings - Fork 8
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
[FE] 인풋 컴포넌트 디자인 수정 및 로그인 페이지 디자인 수정 #417
Conversation
Test Results35 tests 35 ✅ 24s ⏱️ Results for commit 3308c01. ♻️ This comment has been updated with latest results. |
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.
멋쪄요 해리~! 🥹
페이지 디자인이 갈 수록 멋있어지고 있군요!! 디자인 고민 같이 해주고 반영해주셔서 감사합니다 👍
궁금한 사항이랑 수정 요청사항 함께 리뷰 남겼으니 확인 부탁드려요!
font: inherit; | ||
color: inherit; | ||
|
||
appearance: none; /* IOS 디바이스의 버튼 색상이 파란색으로 보이는 문제 해결(@해리) */ |
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.
[P2]
모바일 화면을 살펴보니 select box도 마찬가지로 글씨가 파란색인 것을 확인했습니다. (제가 물론 커스터마이징을 해야하는 것이지만 ㅠㅠ)
커스터마이징 하기 전에는 마찬가지로 반영해야할 듯 싶습니다.
제가 살펴보앗는데 apperance
속성으로는 우측의 화살표만 사라지더라구요. 그래서 아래와 같이 색상을 변경해주면 해결이 되는 것 같아요. 관련 stackoverflow 질문 답변도 같이 첨부해두었습니다.
export const s_dropdown = css`
width: fit-content;
height: 2.8rem;
padding: 0.4rem;
color: ${theme.colors.black};
border-radius: 0.4rem;
`;
before
after
이것도 바꾸어주시면 너무 좋겠네요 해리군~
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.
아하 apperance 속성을 부여하면 색상도 사라지는 군요...! 드롭다운 문제도 같이 해결하도록 할게요.
*/ | ||
height: 6rem; | ||
height: 5.2rem; |
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.
[P2]
제가 이전 PR에서 Funnel 마지막 단계인 날짜 선택 부분에서 BottomFixedButton에 가려지는 문제때문에 임시 div 요소를 추가했는데 해당 높이가 6rem으로 설정되어 있습니다. 한번 보고 같이 해결하면 좋을 듯 싶어요!
노션 링크의 🚀 약속 생성 시, BottomFixedButton에 가려지는 문제 확인 부분 확인하면 됩니다!
floating: css` | ||
padding-top: 1.6rem; /* 텍스트를 아래로 내리기 위해 top padding을 더 줌 (@해리) */ | ||
padding-left: 1.2rem; | ||
|
||
color: #71717a; | ||
|
||
border: none; | ||
${baseInputStyle} | ||
border-radius: 1.2rem; | ||
`, |
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.
이름을 floating으로 지으신 이유가 궁금하네요❗️
보통 <label />
은 <input />
위에 위치하게 되는데 이번 PR에서 생성한 UI의 경우 관습적인 위치에 있는 것이 아니라 <input />
내부에 위치하게 됩니다. 그래서 떠다닌다는 느낌을 명시해 주기 위해서 해당 이름을 선택하게 되었습니다. 처음에는 FloatingLabelInput
으로 사용하려고 했으나 너무 긴 것 같아서 FloatingInput
으로 선택하게 되었지만, 기존의 컴포넌트 이름으로 돌아가는 것이 괜찮을 것 같네요!
현재 placeholder의 색상과 입력 시 색상이 동일합니다. 더 나은 사용자 경험을 위해 입력하는 글자 색상은 검정색으로 유지하는 것이 좋아보여요!
동의합니다!
label: '낙타해리빙봉', | ||
placeholder: '송재석최현웅김윤경', |
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.
샤라웃 오예~ 🥳
transition: box-shadow 0.3s; | ||
|
||
&::placeholder { | ||
color: #71717a; |
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.
[P3]
지금 당장 반영하는 것은 아니지만 우리 색상을 통일하기 위해서 이런 에러 색상이나 placeholder의 색상을 별도의 상수로 지정하는 것이 좋아보입니다!! 시간날 때 가능한 한 빨리요!
const getLabelTextColor = (isFocused: boolean, isError: boolean): string => { | ||
if (isError) return '#EB1E1E'; | ||
|
||
if (isFocused) return theme.colors.pink.medium; |
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.
[Q]
:focus
가상 클래스 선택자를 이용하지 않고 isFocused
를 사용한 이유가 있나요!?
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.
:focus
가상 클래스 선택자는 <input />
태그가 포커스 되었을 때 사용할 수 있는 클래스 선택자입니다! 현재 해당 스타일 로직은 인풋의 스타일을 결정하기 위한 것이 아닌 라벨의 텍스트 색상을 적용하기 위한 로직입니다!
export const s_floatingLabelContainer = css`
position: relative;
display: inline-block;
width: 100%;
/* :focus-within을 사용하여 input이 포커스될 때 부모 컨테이너에 반응 */
&:focus-within label {
color: ${theme.colors.pink.medium};
}
`;
위와 같이 <label />
, <input />
태크를 감싸는 부모 컨데이너 내부에 포커스가 되었을 때 라벨의 스타일을 변경할 수도 있기는 합니다. 그래서, 두 태그를 감싸는 부모 태그에서 에러 상태를 받아서 스타일을 동적으로 결정할 수 있도록 하겠습니다.
즉,
export const s_floatingLabelContainer = (isError: boolean) => css`
position: relative;
display: inline-block;
width: 100%;
color: ${isError ? '#EB1E1E' : '#71717a'};
&:focus-within label {
color: ${isError ? '#EB1E1E' : theme.colors.pink.medium};
}
`;
이렇게 수정하도록 하겠습니다!!
if (!uuid) { | ||
console.error('UUID is missing'); | ||
return; | ||
} |
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.
[P1]
충돌사항 짱많겠네요 🥹🥹🥹🥹🥹🥹🥹🥹🥹
해당 로직은 uuid를 useContext를 사용하게 함으로써 제거되니 참고해주세요 해리군~
바로 rebase하고 반영사항도 함께 보면 좋을 것 같습니다!
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.
이거 머지되면 같이 충돌해결해봐야 할 것 같아요...!
addToast({ | ||
message: TOAST_MESSAGES.OUT_OF_ONE_YEAR_RANGE, | ||
type: 'warning', | ||
duration: 2000, | ||
}); | ||
return; | ||
} |
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.
좋으네요~!
@@ -121,7 +121,7 @@ export const s_rangeStart = (isAllRangeSelected: boolean) => css` | |||
|
|||
position: absolute; | |||
top: 0; | |||
right: 0.4px; | |||
right: 0.02rem; |
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.
[Q]
어째서 0.04rem이 아니라 0.02rem일까요?
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.
@hw0603 페드로와 함께 달력 UI에 대해서 QA를 진행했었는데요..! 0.02rem이 사각형과 삼각형 사이의 간격을 최소화할 수 있는 위치인 것 같아서 이렇게 수정하게 되었습니다!
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.
고생했어요 해리🍀
}; | ||
|
||
// 터치 이벤트를 사용해서 스크롤을 할 경우, 해당 스크롤을 막는다는 것을 브라우저에게 명시적으로 알려주기 위해서 passive 속성 추가(@해리) | ||
document.addEventListener('touchmove', handleTouchMove, { passive: false }); |
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.
오!! passive의 기본값이 이벤트마다 다르군요
- 일반적인 이벤트:
false
- 터치와 스크롤 관련 이벤트 (touchmove, touchstart, wheel 등): 일부 브라우저에서는
true
로 설정
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.
[P3]
FloatingInput
컴포넌트명을 FloatingLabelInput
로 바꾸는 거 어떤가요?
현재 컴포넌트명은 Input이 둥둥 떠다니는 느낌이 듭니다!
참고) 찾아보니 부트스트랩에서는 Floating labels라고 명칭하네요!
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.
저도 해당 컴포넌트명을 생각하기는 했지만, 너무 긴 것 같아서 FloatingInput
으로 줄였는데 빙봉의 의견과 첨부해 준 링크를 확인해 보니 기존의 컴포넌트명으로 롤백하는 것이 더 괜찮을 것 같네요!
frontend/src/constants/button.ts
Outdated
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.
[P3]
이 파일에서 button 관련 상수화 객체들을 정의하고 있으니 파일명을 buttons
로 하는거 어떤가요?
const contentRef = useRef<HTMLDivElement>(null); | ||
|
||
useEffect(() => { | ||
const handleTouchMove = (e: TouchEvent) => { |
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.
[P3]
handleTouchMove
대신 preventTouchMove
라는 함수명 어떤가요?
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.
[P3]
이건 저번 리뷰에서 놓쳤던 부분인데, 갑자기 보이네용🤣
moveToPrevMonth
와 moveToNextMonth
함수에서 new Date(currentYear, currentMonth ± 1)
를 각각 호출하고 있는데, 이 로직이 중복되는 것 같아요.
중복되는 부분을 아래처럼 별도의 함수(ex: moveMonth)로 분리해서 사용해보는 것을 가볍게 제안해 봅니다~!
const moveMonth = (increment: number) => {
const newDate = new Date(currentYear, currentMonth + increment);
setCurrentFullDate(newDate);
};
const moveToPrevMonth = () => moveMonth(-1);
const moveToNextMonth = () => {
const fullDate = getFullDate(currentFullDate);
if (fullDate >= ONE_YEAR_LATER) {
addToast({
message: TOAST_MESSAGES.OUT_OF_ONE_YEAR_RANGE,
type: 'warning',
duration: 2000,
});
return;
}
moveMonth(1);
};
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.
- label이 인풋 태그 내부에 위치하도록 relative, absolute 활용 - focus 상태를 관리
- label과는 다른 역할을 한다는 생각을 바탕으로, FieldLabel과 마크업과 스타일이 동일하지만 추가로 하나의 컴포넌트를 추가로 생성
- 모바일 환경에서만 스크롤을 막으면 되기 때문에, 터치 이벤트로 인한 스크롤만 막도록 구현
- FloatingInput으로 수정 - useAttendeeLogin 커스텀 훅 활용 - 필드가 페이지 상단에 위치하도록 변경
155d745
to
0329517
Compare
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.
👍
관련 이슈
작업 내용
인풋 컴포넌트 디자인 개선
모바일 기기에서는 입력을 하기 위해 가상 키패드가 올라옵니다. 이 때, 가상 키패드 뒤에 가상 영역이 생기는 문제가 있어, 스크롤이 되는데요. 사용자 경험 향상을 위해서 퍼널 패턴을 도입했지만, 아이러니하게도 다시 사용자 경험을 저하시키는 요소가 발견되었습니다. 따라서, 이 문제를 해결하기 위해서 브레인 스토밍을 했었죠!
해당 이미지처럼 닉네임, 비밀번호를 가로로 배치하거나, 닉네임과 비밀번호 입력 스텝을 구분하는 방법이 제시되었는데
이 두 가지 생각으로 인해서 채택되지 않았습니다. 결국 스크롤을 막기 위해서 선택한 방법은 인풋 필드의 높이를 줄이는 것이었습니다.
인풋 필드의 높이를 줄였고, 헤더의 높이도 줄일 예정이므로 모바일 기기에서 현재 포커스되는 인풋 필드로 강제로 스크롤 하는 문제는 어느정도 해결되는 것으로 확인됐습니다. 아이폰 7 사용자는 약간의 스크롤은 발생하기는 하지만, 이전 상황에 비하면 훨씬 개선되었다고 판단했어요.
ScrollBlock.tsx
해당 컴포넌트는 children이 모바일 환경에서 스크롤되지 않도록 하는 책임을 가집니다.
{passive: false}
를 설정이 왜 필요했는지에 대해서 설명드릴게요. 모바일에서 브라우저는 터치 이벤트가 발생할 때, 성능 최적화를 위해서 기본적으로 true로 설정합니다. 마우스나, 손가락을 이용해서 특정 페이지를 스크롤 할 때, 브라우저는 빠르게 이벤트에 반응해서 스크롤을 해줘야 하는 책임을 가지고 있습니다. 이 때, passive 속성은 e.preventDefault()를 실행할지 말지 암묵적으로 결정하는 속성입니다.true인 경우, 브라우저는 이벤트 핸들러가
e.preventDefault()
를 호출하지 않을 것으로 보장받습니다. 그러므로, 이벤트 핸들러가 실행되기 전 우선적으로 스크롤을 처리하게 됩니다. 이것이 스크롤 성능 최적화입니다. 자세한 내용은 MDN의 스크롤 성능 향상 섹션에서 확인할 수 있어요.false인 경우에는, 브라우저가 이벤트 핸들러 내부에서
e.preventDefault()
를 호출할지 모른다고 가정합니다. 따라서, 내부에서 해당 메서드를 호출할 수 있기 때문에 스크롤을 바로 실행하지 않고 이벤트 핸들러가 완료될 때까지 기다립니다. 이 기다림이 메인 스레드를 블록하고, 렌더링 지연으로 이어집니다.브라우저는 스크롤 관련 성능 최적화를 위해서,
touchmove
,wheel
과 같은 이벤트의 passive를 기본적으로 true로 설정합니다. 하지만, 저희는 스크롤을 막기로 결정했기 때문에, false속성과 함께 기본 동작인 스크롤을 막을 수 있도록 e.preventDefault()함수를 호출했습니다!특이 사항
리뷰 요구사항 (선택)