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/tech stack #86

Merged
merged 7 commits into from
Apr 11, 2024
Merged

Fix/tech stack #86

merged 7 commits into from
Apr 11, 2024

Conversation

woo29
Copy link
Contributor

@woo29 woo29 commented Apr 10, 2024

💬 PR 타입 (하나 이상의 PR 타입을 선택해주세요)

  • 기능 추가
  • 기능 변경
  • 기능 삭제
  • 디자인 수정
  • 버그 수정
  • 의존성, 환경 변수, 빌드 관련 코드 업데이트

🌵 반영 브랜치

fix/tech-stack -> dev

🔎 작업 내용

  • 기술 스택 뱃지 프리뷰에서는 가로 1자로 보이지만 깃허브 리드미로 가면 세로 1자로 나오는 버그 수정
  • 테이블과 이미지가 겹치는 버그 수정
  • 기술 스택 뱃지 추가 할때 엔터키로도 가능하게 기능 추가 (휴면 에러(오타) 있을 시 에러 토스트 출력)
  • 기술 스택 뱃지 연관 데이터 키보드 방향키로도 조작 가능하게 기능 추가

✨ 영상

2024-04-10.4.36.17.mov

@woo29 woo29 self-assigned this Apr 10, 2024
Copy link

vercel bot commented Apr 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
readyou-front ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 10, 2024 8:09am

Comment on lines 61 to 73
switch (e.keyCode) {
// UP KEY
case 38:
nowIndex = Math.max(nowIndex - 1, 0);
break;

// DOWN KEY
case 40:
nowIndex = Math.min(nowIndex + 1, matchDataList.length - 1);
break;

// ENTER KEY
case 13:
Copy link
Contributor

@hyjoong hyjoong Apr 10, 2024

Choose a reason for hiding this comment

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

주석으로 keyCode가 뭔지 알려준 것 좋네요 !!
선호도 차이 같은데 저는 주석보다 코드 상단이나 하단에 상수화하는 것을 선호하는데, 참고만 해주세요 ㅎㅎ

const UP_KEY_CODE = 38
const ENTER_KEY_CODE = 13

switch(e.keyCode){
case UP_KEY_CODE:
  nowIndex = Math.max(nowIndex - 1, 0);
  break;

}

Comment on lines 105 to 110
const newMacthList = matchDataList.slice(0, 4).map((data, index) => {
if (index === nowIndex) {
return { text: data, index, isOver: true };
}
return { text: data, index, isOver: false };
});
Copy link
Contributor

Choose a reason for hiding this comment

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

이 코드만 봤을 때 왜 앞에 4개만 잘라서 map()메서드를 사용하는지 모르겠네요 제가 생각하는 이유는 기술스택 검색 추천 리스트를 첫 4가지 요소에 대한 옵션을 제공 하기 위해서 같은데 맞을까요 ?? 코드 상단에 주석으로 이 로직이 의미하는 이유를 작성해주시면 좋을 것 같아요

// 기술스택 검색 추천 리스트를 첫 4가지 요소에 대한 옵션을 제공
const newMatchList = matchDataList.slice(0, 4).map((data, index) => ({
  text: data,
  index,
  isOver: index === nowIndex // 이런 식으로도 작성 가능하겠네요!
}));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

지금은 데이터가 적어서 괜찮을 것 같은데 나중에 데이터가 많으면 한 글자 입력하면 연관 데이터가 너무 많이 나와서 카톡으로 얘기해보고 상단 4개만 거르자고 해서 slice했던 것 같네요 주석도 달아 놓을게요!!

Copy link
Contributor

@hyjoong hyjoong Apr 10, 2024

Choose a reason for hiding this comment

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

아하 그렇군요 ㅎㅎ 저도 기술스택 검색시 상단에 4개만 추천하는 것을 카톡으로 얘기한 것을 알고있었지만
이코드가 톡에서 얘기한 기술스택 4개 표시하는 부분인지 모를수도 있어서 였습니다!

바로 반경 감사합니다~

Comment on lines 28 to 32
{
text: '',
index: 0,
isOver: false,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

기존에 빈 배열의 데이터를 가지고 있었다가 초기값에

   {
      text: '',
      index: 0,
      isOver: false,
    }

데이터를 추가하신 이유가 어떻게 될까요??

Copy link
Contributor Author

@woo29 woo29 Apr 10, 2024

Choose a reason for hiding this comment

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

객체처럼 조금 복잡한 데이터가 들어오면 어떤 형식으로 들어올거다라고 알려주는게 습관되어있던 것 같네요 근데 위에 타입으로 보여서 이것도 지우는걸로 반영하겠습니다~!

@gkfla668 gkfla668 merged commit 64ae862 into dev Apr 11, 2024
2 checks passed
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.

4 participants