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

Input 컴포넌트 자동완성용 데이터 삽입 & 요소 선택 시 input에 입력값 저장하기 구현 #3

Merged
merged 6 commits into from
Feb 15, 2023

Conversation

userJu
Copy link
Collaborator

@userJu userJu commented Feb 14, 2023

📄 구현 내용 설명

  • Input 컴포넌트 자동완성용 데이터를 삽입했습니다
  • Input 창에서 요소를 선택할 시 input에 입력값이 계속 쌓이며 저장되게 구현했습니다

⛱️ 주요 변경 사항

  • 컴포넌트와 하위 로직을 분리하기 위해 utils 폴더를 만들어 simple-icons를 가져와 배열로 변환해주는 코드를 작성했습니다.
  • Autocomplete 태그에 filterSelectedOptions를 추가해 선택된 요소가 input에 쌓이게 만들었습니다.
  • input에 쌓인 요소의 title을 selectedIconNames에 저장했습니다 @msdio 가 메인페이지를 구현할 때 쓸 수 있을 것 같습니다👍

🙋 이 부분을 리뷰해주세요

  • utils 내부에 getAllIconInfo.ts를 삽입한 것이 적절한 구조인지 조금 의문이 듭니다
  • import시 eslint 오류가(Run autofix to sort these imports!eslint[simple-import-sort/imports](https://github.com/lydell/eslint-plugin-simple-import-sort#sort-order)) 거듭 발생해
    "simple-import-sort/imports": "error",
    "simple-import-sort/exports": "error",

이 부분을 주석 처리하고 진행했습니다. issue에 따로 작성하겠습니다

  • Encountered two children with the same key, Handshake`` 에러가 발생합니다. 동일한 key를 가지고 있는 중첩되는 요소가 있는 것 같습니다. 이 내용도 issue에 작성하겠습니다!

🤔 여기를 참고하면 도움이 돼요

  • [링크 이름](링크 주소)

객체로 되어있는 Icon 정보를 가져오는 함수와 Icon 정보를 배열로 만드는 함수를 분리했다.
Icon을 가져오지 못했을 때 에러 처리와 함수의 역할 최소화를 위해 분리함
@msdio
Copy link
Owner

msdio commented Feb 15, 2023

utils 내부에 getAllIconInfo.ts를 삽입한 것이 적절한 구조인지 조금 의문이 듭니다

저는 getAllIconInfo 함수에 에러 처리를 해놓은 이유가 궁금해요! 아이콘 정보를 가져오지 못하는 경우가 언제일까요?

@@ -0,0 +1,16 @@
import * as icons from 'simple-icons';
Copy link
Owner

Choose a reason for hiding this comment

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

👍

src/App.tsx Outdated
<img height='32' width='32' src='https://cdn.simpleicons.org/react' />
<img height='32' width='32' src='https://cdn.simpleicons.org/javascript' />
<img height='32' width='32' src='https://cdn.simpleicons.org/spring' />

<Input></Input>
<Home></Home>
Copy link
Owner

Choose a reason for hiding this comment

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

저는 children이 없는 경우에는 <Home /> 과 같이 self-closing tag를 사용하는 편인데, 나중에 다른게 들어갈 수 있어서 이렇게 작성하신 걸까요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 저 부분은 언제든지 수정이 가능한 부분이라 고민 없이 저렇게 작성했어요! 셀프 클로징으로 변경하겠습니다!

@msdio
Copy link
Owner

msdio commented Feb 15, 2023

utils 내부에 getAllIconInfo.ts를 삽입한 것이 적절한 구조인지 조금 의문이 듭니다

simple icon 서버에서 아이콘들을 가져온다라는 관점에서는 services 폴더 같은 곳으로 빼도 괜찮을 것 같고,
아이콘을 가져와서 우리가 쓸 수 있게 바꿔서 리턴해준다 라는 관점에서는 지금 구조도 좋은 것 같아요!

@userJu
Copy link
Collaborator Author

userJu commented Feb 15, 2023

utils 내부에 getAllIconInfo.ts를 삽입한 것이 적절한 구조인지 조금 의문이 듭니다

저는 getAllIconInfo 함수에 에러 처리를 해놓은 이유가 궁금해요! 아이콘 정보를 가져오지 못하는 경우가 언제일까요?

simple-icons 자체에 문제가 생기지 않는 이상 일어나지 않을 것 같아서 저도 저 에러 처리를 하는게 맞나 고민했었어요! 괜히 두 번의 함수를 거쳐야 하는 번거로움이 있을 것 같으니 icon 정보를 받아 배열로 만들어 return 해주는 함수만 남겨둘게요!

@userJu
Copy link
Collaborator Author

userJu commented Feb 15, 2023

아이콘을 가져와서 우리가 쓸 수 있게 바꿔서 리턴해준다

그러면 지금은 이대로 utils 내부에 두고 나중에 폴더 구조를 변경할 사항이 있으면 다시 논의해봐도 좋을 것 같습니다 😻 리뷰 고마워요 하루!

@msdio msdio merged commit cb57bd0 into main Feb 15, 2023
@userJu userJu deleted the feature/inputLogic branch February 15, 2023 10:18
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