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: duplicate class name in suffix icon #97

Merged
merged 6 commits into from
May 16, 2024
Merged

Conversation

Hanna922
Copy link
Member

@Hanna922 Hanna922 commented May 14, 2024

1️⃣ 어떤 작업을 했나요? (Summary)

버그 픽스

  1. className="suffix-icon"이 중복으로 발생하기 때문에, suffix icon을 설정하는 TextField를 2개 이상 사용할 경우 suffix-icon이 정상적으로 렌더링되지 않는 문제가 발생합니다.
    => 기존 div tag를 styled-components로 설정해줌으로써 class name을 랜덤으로 설정하게 됩니다.
수정 전 수정 후
image image
  1. 작업 하면서 다른 boolean 속성들도 undefined로 설정되는 문제가 있다는 것을 발견했습니다. (isMarked와 동일)
    => 기본 값을 모두 설정해주었습니다.

2️⃣ 알아두시면 좋아요!

수정 하면서 포커스가 되어 있지 않아도 아이콘이 계속 보이도록 해두었는데, 디자인 소통이 계속 지연되는 것 같아서 우선 이렇게 개발해두는 게 어떠신가요? 포커스 여부에 따라 visibility 속성을 수정해주는 로직이 추가되어야 한다는 이유와 아무리 봐도 이게 더 자연스러운 것 같다는 2가지 이유입니다.

3️⃣ 추후 작업

1.1.1 배포

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?

@Hanna922 Hanna922 added bug Something isn't working fix labels May 14, 2024
@Hanna922 Hanna922 self-assigned this May 14, 2024
@Hanna922 Hanna922 requested a review from nijuy as a code owner May 14, 2024 13:11
@Hanna922
Copy link
Member Author

Hanna922 commented May 14, 2024

SuffixTextField의 suffix 속성을 string이 아닌 React.ReactNode로 설정해달라는 니즈가 있는 상황인데(숨쉴 #182) 개인적으로 저희 YDS 코드 단에서는 ReactNode로 둬도 좋을 것 같다는 생각이 드네요.
@nijuy 어떻게 생각하시나요? 괜찮으시면 커밋 추가할게요

@nijuy
Copy link
Collaborator

nijuy commented May 15, 2024

(제가 수업 중에 다는 거라 다른 부분은 수업 마치고 2시 전으로 마저 확인하겠습니다)

SuffixTextField의 suffix 속성을 string이 아닌 React.ReactNode로 설정해달라는 니즈가 있는 상황인데(숨쉴 #182) 개인적으로 저희 YDS 코드 단에서는 ReactNode로 둬도 좋을 것 같다는 생각이 드네요. @nijuy 어떻게 생각하시나요? 괜찮으시면 커밋 추가할게요

이슈 확인했습니다!! 저는 suffix의 목적을 입력값 뒤에 붙일 문자열로만 생각하고 있었는데,
Soomsil에서의 사용 케이스를 보면 꼭 그런 용도만은 아닌 거 같아서 말씀하신 방향으로 수정하는 게 좋겠네요!!

Copy link
Collaborator

@nijuy nijuy left a comment

Choose a reason for hiding this comment

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

작업 하면서 다른 boolean 속성들도 undefined로 설정되는 문제가 있다는 것을 발견했습니다. (isMarked와 동일)

넵! 다른 컴포넌트 작업할 때도 참고해야겠네요 숙지하겠습니다~~


수정 하면서 포커스가 되어 있지 않아도 아이콘이 계속 보이도록 해두었는데, 디자인 소통이 계속 지연되는 것 같아서 우선 이렇게 개발해두는 게 어떠신가요?

저는 선개발 후통보는 반대입니다 ㅠㅠ
들어주신 구체적인 이유에 대한 제 의견, 대안은 아래에 정리 해두었습니다

디자인 팀과의 협의가 진행되지 않고 있는 상황인 점 알지만 (저도 애타게 기다리는 중)
그렇다고 웹에서 독단적으로 개발을 끝내두면 분명 나중에 혼란이 생길 거 같아요
나중에 다른 사람이 피그마와 동일하지 않아 오류라고 판단하거나, 이렇게 개발하게 된 히스토리 추적을 해야한다거나요.

모바일 YDS와의 동작도 달라지는 것도 염려가 됩니다. (이건 모바일과 웹 YDS가 완전히 동일하게 동작해야 하는지 자체에 대한 궁금증이 생겨서 좀 더 고민해보겠습니다. 이게 주된 이윤 아니지만 일단 넣었어요~~..)

내일이 디자인팀 모함디니까 오늘 한번 더 슬랙 보낼게요
저희 문의를 담당할 팀원을 한분 정해주셨으면 좋겠긴 하네요 🤔


1. 포커스 여부에 따라 visibility 속성을 수정해주는 로직이 추가되어야 한다

기존 방식 + 아이콘 컨테이너를 스타일 컴포넌트로 정의한 방식을 섞으면 별도의 로직을 고안하지 않아도 될 거 같습니다! (섞는다는 건 아래 코드를 말합니다)

export const StyledTextFieldWrapper = styled.div<StyledTextFieldProps>`
   input:focus + ${StyledSuffixIconContainer} {
    // code
  }
`;

2. 아무리 봐도 이게 더 자연스러운 것 같다

입력 후 Focus가 없어졌을 때가 아니라 입력 여부와 무관하게 노출된다는 점에서 이전에 제안하신 방식과 약간 달라졌네요!
저는 내용이 없을 때는 기존처럼 x 버튼이 노출되지 않는 쪽이 더 자연스럽다고 생각해요.
좀 더 구체적으로 말하자면 어차피 눌러도 무용한 버튼은 굳이 노출하지 않는 게 시각적 잡음을 줄일 수 있다! 쪽에 가깝겠네요

@Hanna922
Copy link
Member Author

@nijuy 말씀해주신 '기존 방식 + 아이콘 컨테이너를 스타일 컴포넌트로 정의한 방식을 섞기' 방법 사용 시 suffix icon을 클릭 했을 때 focus가 일시적으로 풀려, onClick 동작이 정상적으로 진행되지 않는다는 문제점이 있습니다 ㅠㅡㅠ (제 문제일수도)
굉장히 많은 시도를 해봤는데,, 방법을 모르겠네요ㅠ 일단 제가 마지막으로 시도해본 커밋 d5d31e2 올려두었습니다
(커밋 로그가 섞여서 다시 올렸습니당)

@Hanna922
Copy link
Member Author

@nijuy 대박사건. 샤워 하면서 보리의 active 코드가 번뜩 생각났는데, 원인이 맞았어요. 해결 완 ^~^

Copy link
Collaborator

@nijuy nijuy left a comment

Choose a reason for hiding this comment

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

제가 요즘 확인이 너ㅜ뮤 늦네요................................... 반성중
코멘트가 있긴 한데 큰 수정은 아니라 코드 제안으로 넣어놔서 어푸룹 했스빈다!!

src/components/TextField/TextField.style.ts Outdated Show resolved Hide resolved
Co-authored-by: 이유진 <youjin6325@naver.com>
@Hanna922 Hanna922 merged commit f48f4e2 into develop May 16, 2024
@Hanna922 Hanna922 deleted the hotfix/#96-remove-icon branch May 16, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: remove suffix icon
2 participants