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] TextInput 공통 컴포넌트 구현 #126

Merged
merged 18 commits into from
Nov 10, 2024
Merged

Conversation

simeunseo
Copy link
Collaborator

@simeunseo simeunseo commented Nov 9, 2024

관련 이슈 번호

작업 내용

  • TextInput 컴포넌트 구현
  • label, description, required 값에 따른 조건부 렌더링
  • errorMessage가 존재할 경우 렌더링
  • maxLength가 존재할 경우 글자수 카운터 렌더링 (이 부분은 현재 피그마에는 반영되어있지 않으나 티클 개설 폼에서 필요할 것 같아서 추가했습니다)
  • 웹 접근성을 고려한 속성 추가

PR 포인트

  • [중요] 에러 메시지를 표시할 때 에러 아이콘을 띄워야하는데, 아직 svg 사용 방식에 대한 합의가 이루어지지 않아서 일단 제외하고 구현을 했습니다. 금일 내로 svg 사용 방식에 대해 논의가 될 수 있으면 좋겠습니다!! -> svgr 사용하는 것으로 논의 완료
  • 원활한 리뷰와 머지를 위해 cn util 구현 PR을 따로 분리했는데, 이 작업에서 바로 사용해야해서 불가피하게 branch chaining을 해서 작업했습니다.
  • 기존에 textInput이 hover시에 background color가 옅은 보라색으로 변경되도록 디자인이 되어있었는데, hover할때마다 색상이 깜빡깜빡 바뀌는게 불필요할 것 같아서 제거를 했습니다..!
  • TextInput이 form에 사용되는 형태를 고려하여, 기본적으로 width 100%를 적용하고 width 설정이 필요하다면 className을 주입해서 사용할 수 있도록 했습니다.
  • global.css에 색상 변경에 대한 transition을 추가했는데 혹시 다르게 적용하고 싶으시면 말씀해주세요!
      * {
        @apply transition-colors duration-200 ease-in-out;
      }

고민과 학습내용

공통 컴포넌트를 나누는 기준

TextInput과 유사한 UI 및 동작을 가지고있는 SearchInput, Textarea 컴포넌트를 통합해서 구현할 수 없을까?에 대해 고민을 했습니다!
공통 컴포넌트를 나누는 기준

실제 기업의 디자인 시스템을 참고하고, 세가지 컴포넌트의 특성과 확장성을 고려했을 때
결론적으로 세가지 컴포넌트를 모두 따로 구현하기로 결정했습니다.

웹 접근성을 고려한 속성

  • label의 htmlFor과 input의 id 속성을 통해 label이 가리키는 input을 연결지었습니다.
  • 필수 항목을 나타내는 *표의 경우 스크린 리더가 읽지 않도록 aria-hidden 옵션을 활성화했습니다.
  • aria-invalid 속성으로 에러가 발생한 input에 대해 입력값이 올바르지 않음을 표시했습니다.
  • aria-describedby 속성으로 에러가 발생한 input에 대해 에러메시지를 연결지었습니다.
  • aria-describedby 속성으로 input에 대한 description을 연결지었습니다.

스크린샷

기본 focus 및 placeholder

image

모든 옵션을 적용

image

2024-11-10.8.14.09.mov

-> 영상에서 border color가 일부만 적용된 것처럼 보이는데 실제로 보면 그렇지 않습니다!

@simeunseo simeunseo added FE Frontend 관련 작업 ✨ Feature 새로운 기능 추가 labels Nov 9, 2024
@simeunseo simeunseo self-assigned this Nov 9, 2024
@github-actions github-actions bot added the size/l label Nov 9, 2024
Comment on lines 11 to 12
default: 'border-main focus:border-primary',
error: 'focus:border-error',
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3:
property key에 경우 따로 상수로 정의하여 사용하는 것이 좋아보입니다.

Suggested change
default: 'border-main focus:border-primary',
error: 'focus:border-error',
const KEY = {
default: "default",
error: "error"
}
[KEY.default]: 'border-main focus:border-primary',
[KEY.error]: 'focus:border-error',
...
const inputState = errorMessage ? KEY.error : KEY.default;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제안 감사드립니다!!
KEY보다 조금 의미있는 네이밍이면 좋을 것 같아서 STATE로 정의했는데 괜찮을까요? 컨벤션 맞추면 좋을거같습니다!

const STATE = {
  default: 'default',
  error: 'error',
} as const;

@github-actions github-actions bot added size/m and removed size/l labels Nov 10, 2024
@simeunseo
Copy link
Collaborator Author

simeunseo commented Nov 10, 2024

@seoko97 svg 세팅 완료했습니다~!
개인적으로 barrel 파일은 굳이 필요 없어보여요.
src/assets/icons에 snake-case로 svg 파일 저장해오고
사용할 때는 PascalCase + Ic Suffix로 네이밍 컨벤션 맞춰서 import하여 사용하면 될 것 같은데 어떠신가요?

├── assets
│   └── icons
│       └── exclamation.svg
import ExclamationIc from '@/assets/icons/exclamation.svg?react';

@simeunseo simeunseo requested a review from seoko97 November 10, 2024 08:17
Copy link
Collaborator

@seoko97 seoko97 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 말씀하신대로 svg import시 Ic 라는 키워드를 붙이도록 하겠습니다!

Comment on lines 7 to 10
const STATE = {
default: 'default',
error: 'error',
} as const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3:
하위에서 선언하는 변수에 이름이 inputState 여서 네이밍 자체는 문제가 없는 것 같습니다! 다만 React의 state와 혼용될 수 있으니 TYPE 또는 STATUS로 변경하는 것이 좋을 것 같습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 완전 동의합니다 ㅠ
좀 더 생각을 해봤는데, 실제로 이 variant의 의미가 무엇인지를 생각해보니 validation에 관련된 값(default, error)이거든요.
그래서 VALIDATION_STATE라는 이름으로 통일을 했습니다.

const VALIDATION_STATE = {
  default: 'default',
  error: 'error',
} as const;

const inputVariants = cva(
  'w-full rounded-base border bg-white px-3.5 py-2.5 text-body1 text-main placeholder:text-weak',
  {
    variants: {
      validation: {
        [VALIDATION_STATE.default]: 'border-main focus:border-primary',
        [VALIDATION_STATE.error]: 'focus:border-error',
      },
    },
    defaultVariants: {
      validation: 'default',
    },
  }
);

...

 const inputValidation = errorMessage ? VALIDATION_STATE.error : VALIDATION_STATE.default;

그래서 이런식으로 각 스타일링의 의미가 무엇인지에 따라서 각자 적절한 네이밍을 붙이면 좋을 것 같습니다!

@simeunseo simeunseo merged commit fcb1a09 into develop Nov 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE Frontend 관련 작업 ✨ Feature 새로운 기능 추가 size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] TextInput 공통 컴포넌트 구현
3 participants