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

[디자인시스템] 3주차 변성진 #9

Open
wants to merge 3 commits into
base: seounjin
Choose a base branch
from

Conversation

seounjin
Copy link
Collaborator

구현내용

Modal 컴포넌트

  • compound component 패턴을 사용하여 구현
  • Header, Body, Footer를 나누어서 조합하는 형식으로 구현함
  • 접근성 속성 추가

Combobx 컴포넌트

  • 기존 Select 컴포넌트에 Input을 확장시켜 구현

고민한점

공통

파일을 만들때 prefix Naming convenstion을 사용할지 dot notation convenstion을 사용해야 하는게 좋을지 고민함

Combox 컴포넌트

  • 새로운 Combobox컴포넌트를 만들지 기존의 Select컴포넌트에서 확장시킬지 고민함
  • itemList를 Combox 컴포넌트안에서 제어할지, 사용자가 직접 제어하게 할지 고민함
  • input의 onChange 핸들러는 밖에서 제어하게할지 안에서 제어하게 할지 고민함
  • onSelect(value) value값을 string 고정값이 아닌 여러가지 타입이 들어가게 만들기위에 Select컴포넌트를 제네릭타입을 사용하여 만들려고 하였으나 onSelect에 타입에러를 해결하지 못하여 구현하지 못함. (보통은 string값 혹은 정해진 값으로 고정시키나요 ㅇ?)

구현하려고 했던 코드

export interface SelectProps<T> {
  children: ReactNode;
  defaultValue?: string;
  itemList?: T[];
  onSelect: (props: T) => void;
  onInputChange?: (event: ChangeEvent<HTMLInputElement>) => void;
}

const Select = <T extends unknown>({
  children,
  defaultValue,
  itemList,
  onSelect,
  onInputChange,
}: SelectProps<T>) => {
  const { isOpen, option, setIsOpen, setOption } = useSelect({ defaultValue });

  return (
    <SelectContext.Provider
      value={{
        isOpen,
        option,
        itemList,
        setOption,
        setIsOpen,
        onSelect, // 에러발생!!!
        onInputChange,
      }}
    >
      <S.Container width="280px" height="56px">
        {children}
      </S.Container>
    </SelectContext.Provider>
  );
};

export default Select;

Select.Trigger = Trigger;
Select.OptionList = OptionList;
Select.Option = Option;
Select.Input = Input;

export interface SelectContextTypes<T> {
  isOpen: boolean;
  option: string;
  itemList?: Array<string>;
  setIsOpen: Dispatch<SetStateAction<boolean>>;
  setOption: Dispatch<SetStateAction<T>>;
  onSelect: (props: T) => void;
  onInputChange?: (event: ChangeEvent<HTMLInputElement>) => void;
}

export const SelectContext = createContext<SelectContextTypes<unknown> | null>(
  null
);

error

Type '(props: T) => void' is not assignable to type '(props: unknown) => void'.
Types of parameters 'props' and 'props' are incompatible.
Type 'unknown' is not assignable to type 'T'.
'unknown' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'unknown'.ts(2322)

return isOpen ? (
<S.Layout
role="dialog"
aria-labelledby="modal-title"
Copy link
Contributor

Choose a reason for hiding this comment

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

<div role="dialog" aria-modal="true" aria-labelledby="dialog_label" aria-describedby="dialog_desc">
  <h2 id="dialog_label">모달 제목</h2>
  <div id="dialog_desc">모달 내용 설명입니다.</div>
</div>

보통 이렇게 id랑 짝꿍인 aria-속성이 들어가면 그에 맞는 id가 기대되는데
modal-title 에 맞는 id는 없는 것....같아요? 👀

Comment on lines +18 to +30
const [isOpen, setIsOpen] = useState<boolean>(false);

const onConfirm = () => {
setIsOpen(false);
};

const onOpen = () => {
setIsOpen(true);
};

const onClose = () => {
setIsOpen(false);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

보통 open, close 같은 열림 상태 관리나, 사용자가 닫을 수 있는 방법을 공통화해놓으면 사용자가 여러군데에서 사용할 수 있더라구요. (chakra에서는 useDisclosure 라는 훅으로 사용하고 있어요)

interface ModalLayoutProps extends PropsWithChildren {}

const ModalLayout = ({ children }: ModalLayoutProps) => {
const { isOpen, onClose } = useModalContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

useModal()(예시) 같은 훅을 하나 만들어서, 거기서 isOpen, onClose 같은 속성을 관리하고,
추가로 모달은 키보드 인터랙션(ex) esc를 누르면 꺼진다, overlay를 누르면 꺼진다) 같은 것도 같이 관리하면 좋을 것 같아요.

Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 키보드 이벤트가 나와서 참고로 말씀드리면,

운영체제마다 키보드의 event.key 값이 조금씩 다른거 아시나요?
예를들어, 맥이랑 윈도우에서 어디는 스페이스바 이벤트 키가 어디는 event.key === 'Space' 이고, 어디는 event.key=' ' 입니다 ㅎㅎ
그래서 esc를 눌러도 event.key==='Esc' 일수도, event.key==='Escape' 일 수도 있습니다!

이런 점 참고해서 키보드 인터랙션도 구현하시면 좋을 것 같아요

Comment on lines +17 to +19
<S.Content role="document" tabIndex={-1}>
{children}
</S.Content>
Copy link
Contributor

Choose a reason for hiding this comment

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

tabIndex를 넣으신 이유가 있을까요??
스토리북상으로 확인했을 땐 tabIndex={-1} 이 있을 때랑, 없을 때랑 차이가 없는 것...같아요...?

import { createPortal } from "react-dom";

const ModalPortal = ({ children }: PropsWithChildren) => {
const container = document.getElementById("modal");
Copy link
Contributor

Choose a reason for hiding this comment

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

모달용 포탈을 만들어주셨군요! 👍

@@ -16,7 +19,11 @@ const Option = ({ children, value }: OptionProps) => {
setIsOpen(false);
};

return <S.OptionItem onMouseDown={handleOption}>{children}</S.OptionItem>;
return (
<S.OptionItem id={`listbox-option-${id}`} onMouseDown={handleOption}>
Copy link
Contributor

Choose a reason for hiding this comment

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

키보드 화살표 위아래로도 뭔가 요소를 선택할 수 있으면 더 좋을 것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

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

React에 빌트인된 useId라는 훅이 있습니다 ㅎㅎ 걔를 활용해보셔도 좋을 것 같아요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants