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

feat: TextField의 suffix 관련 props 제거, docs 수정 #70

Closed
Hanna922 opened this issue Apr 23, 2024 · 5 comments · Fixed by #72
Closed

feat: TextField의 suffix 관련 props 제거, docs 수정 #70

Hanna922 opened this issue Apr 23, 2024 · 5 comments · Fixed by #72
Assignees
Labels

Comments

@Hanna922
Copy link
Member

Hanna922 commented Apr 23, 2024

ISSUE ✅ : TextField의 suffix 관련 props 제거, docs 수정

📖 Summary

피쳐로 분리하는 이유: 사용처 코드 변경이 있을 수 있음

고민한 부분

SimpleTextField에 suffix를 @soongsil.ac.kr로 넣어주면 SuffixTextField와 동일합니다.
TextField가 너무 많은 역할을 지니고 있다고 생각하여, suffix 관련 props를 모두 제거합니다. => 요거 고민중

SimpleTextField, SuffixTextField, PasswordTextField는 유어슈 웹 팀의 편의를 위해 suffix를 특정 값으로 고정시키며
(SuffixTextField의 경우 예외를 두어 사용자가 string 값을 넣을 수 있도록 합니다.) => 요거 고민중
사용자가 자유로운 커스텀을 원한다면 TextField를 확장할 수 있도록 export 합니다.

변경사항

  • TextField의 searchPrefix props 제거
  • SimpleTextField suffix 고정
  • TextField export
  • SimpleTextField 내부의 공통 TextField 문서 분리
@Hanna922 Hanna922 added the feat label Apr 23, 2024
@Hanna922 Hanna922 self-assigned this Apr 23, 2024
@nijuy
Copy link
Collaborator

nijuy commented Apr 23, 2024

지난 번 SuffixTextField 관련 pr을 읽고 저도 더 고민하던 중이었는데요.
본문 읽어보니 초기 TextField 작업에서 고려했던 방향과 많이 달라지는 거 같기도 하고, 이해하는 바가 다른 부분도 존재하여 일부나마 코멘트로 달아둡니다.

SimpleTextField에 suffix를 넣으면 SuffixTextField와 동일해진다.

SimpleTextField 사용시 suffix 커스텀이 가능하도록 구현해야 했다면 SuffixTextField와 역할이 중복되므로 문제가 되는 상황이 맞습니다.

그러나 이전에 말씀 드렸듯이 현재 suffix 커스텀이 가능한 건 의도가 아니라 props 순서에 의해 발생하는 구현 상의 버그이고, 이번주 내로 불가하도록 수정될 예정이니 컴포넌트 간 역할 중복은 발생하지 않게 됩니다.

SuffixTextField는 suffix를 특정 값으로 고정한다.

SuffixTextField는 suffix 커스텀을 위해 존재하는 컴포넌트라고 이해했습니다. 아마 문서에도 설정 가능하다고 적어둔 상태일텐데요 (이 부분은 핸드폰으로 확인이 어려워서 이따 확인해볼게요 🥸) 사용자가 suffix props에 string을 넣을 수 있는 게 예외적인 허용이라는 부분이 잘 이해가 되지 않습니다.

제가 이동 중이라 조금 있다가 나머지도 달아둘게요~

@Hanna922
Copy link
Member Author

@nijuy 고민 중이라 적혀있는 부분은 아직 결정된 부분이 아닙니다.. 코멘트는 픽스 후 재요청 드려도 괜찮을까요? 의견은 환영입니다

@nijuy
Copy link
Collaborator

nijuy commented Apr 23, 2024

제가 내일 밤까지는 빠른 확인이 어려울 거 같아서 마음이 급했네요 죄삼다 🥺
다만 위 코멘트는 고민 내용에 대한 의견보다는 다르게 알고 있는 부분에 대한 코멘트입니당

(SuffixTextField) 사용자가 suffix props에 string을 넣을 수 있는 게 예외적인 허용이라는 부분이 잘 이해가 되지 않습니다.

특히 이 부분은

SimpleTextField, SuffixTextField, PasswordTextField는 유어슈 웹 팀의 편의를 위해 suffix를 특정 값으로 고정시키며 (SuffixTextField의 경우 예외를 두어 사용자가 string 값을 넣을 수 있도록 합니다.)

를 읽고 SuffixTextField에서 suffix는 고정된 값이 아닌데..? 라는 의문이 들어서 적은 내용인데요.
혹시 아래를 의미하신 게 맞나요??

SimpleTextField, SearchTextField, PasswordTextField는 유어슈 웹 팀의 편의를 위해 suffix를 특정 값으로 고정시키며 (SuffixTextField의 경우 예외를 두어 사용자가 string 값을 넣을 수 있도록 합니다.)


TextField export 같은 사항에 대한 제 생각은 말씀하신대로 추후에 달겠지만,
예정에 없던 작업인데 다른 사람이 작업했던 부분이 수정 범위에 들어간다면! 최대한 논의 후에 진행했으면 합니다 ㅠㅠ
애초에 그럴 예정이었던 건데 제가 오해했다면 알려주세요 🤔 아님 논의용 이슈에 달 라벨을 하나 만들어도 조을 거 같네요

@Hanna922
Copy link
Member Author

Hanna922 commented Apr 23, 2024

네네 맞아요 SuffixTextField는 수정 가능하다!를 전달하고 싶었던 건데 같은 문장에 들어있는 게 오해를 만들었네요 ㅠㅡㅠ
아직 작업 전이고, searchPrefix props 제거 후 PasswordTextField 작업 하다보니 고민이 겹쳐서 먼저 작성해둔 고민 이슈 맞습니다. 라벨 하나 만드는 거 좋은 것 같네요

  • 지금 다시 생각하니까 TextField 내보내는 것도 적절하지 않은 것 같아서,, 좀 더 고민해보고 저희 내일 밤이나 목요일에 얘기 한 번 나눠보면 좋을 것 같아요 (슬랙으로 더 얘기해요!)

@nijuy
Copy link
Collaborator

nijuy commented Apr 27, 2024

(TextFieldProps와 문서 분리에 대한 내용은 #72 코멘트로 달았습니다)

SimpleTextField suffix 고정

이건 제가 SimpleTextField 수정하면서 고쳐야 할 버그이니 해당 이슈에서 고려하지 않으셔도 될 듯 합니다~~!

TextField export ➡ 반대

우선 저는 TextField를 용도별 구현을 위해 사용하는 베이스 컴포넌트라고 계획하고 초기 구현을 진행했고, 이를 export 했을 때 예상하는 문제점은 아래와 같습니다.

  • 같은 목적을 사람마다 다른 방법으로 구현할 여지가 있다

    A: SuffixTextField를 사용
    B: TextField에 적당한 props를 넣고 SuffixTextField처럼 사용

    장기적으로 봤을 때 사용법에 대한 혼란을 일으킬 수 있고, 저희가 YDS를 사용하는 모든 TF에 참여할 게 아니므로 예시로 든 케이스가 발생했을 때의 관리도 어렵다고 생각합니다. 문서에 충분한 설명을 기재한다고 해도 모두가 꼼꼼히 읽는다는 보장이 없어서 문서만으로 이를 막기가 어려울 거 같으네요

  • 디자인을 위반하는 TextField 사용을 허용한다

    TextField는 모든 기능을 포함하고 있기 때문에 searchPrefix suffix를 동시에 넣는다거나 하는 케이스가 생길 수 있을 거 같네요. 이걸 '자유로운 커스텀'으로 볼 수도 있겠지만 예측했던 범위 (예: searchPrefix는 suffix와 동시에 존재할 일이 없음)를 벗어나므로 어떤 문제가 생길지 알 수 없다는 점을 고려해서 '위반'이라고 표현했습니다

    저는 정해진 용도 외의 사용은 YDS에서 지원할 게 아니라 해당 프로젝트에서 자체 구현하는 게 맞다고 봅니다. 디자인팀은 4가지 TextField가 존재한다는 점만 인지하고 계실 테니 용도 외의 TextField 사용이 요구되는 일이 있을까? 싶기도 한데, 이건 뭐 사람 일 모르는 거니까...........

    그럼 여기서 TextField가 모든 기능을 포함하지 않게 수정하면 되지 않느냐? 라는 의문이 드실 거 같은데, 이에 대한 의견은 refactor: text field props #72 에서 충분히 적은 거 같아 해당 이슈에서는 링크만 걸어두겠습니다.

Hanna922 added a commit that referenced this issue Apr 29, 2024
Merge branch 'develop' into feat/#70-TextField-props
Hanna922 added a commit that referenced this issue Apr 29, 2024
* docs: separate TextField basic docs

* @Hanna922
Merge branch 'develop' into feat/#70-TextField-props

* docs: add Controls in base TextField
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants