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: Card component #53

Merged
merged 9 commits into from
Nov 30, 2024
Merged

feat: Card component #53

merged 9 commits into from
Nov 30, 2024

Conversation

Sunjae95
Copy link
Contributor

@Sunjae95 Sunjae95 commented Nov 6, 2024

체크리스트

  • 이슈가 연결되어 있나요?

@Sunjae95 Sunjae95 self-assigned this Nov 6, 2024
@Sunjae95 Sunjae95 linked an issue Nov 6, 2024 that may be closed by this pull request
@Sunjae95 Sunjae95 marked this pull request as ready for review November 6, 2024 15:06
@Sunjae95 Sunjae95 requested a review from a team as a code owner November 6, 2024 15:06
Comment on lines 8 to 15
interface CardProps {
id: string;
name: string;
cohort: number;
grade: "SEED" | "SPROUT" | "SMALL_TREE" | "BIG_TREE";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

props는 #28 참고하여 임시로 추가했습니다.
위의 작업이 merge되면 수정하겠습니다.

<li className={styles.item}>
<img src={imageTable[grade]} alt={`${grade} image`} />
<section>
<section aria-labelledby={`card-github-${id}`}>
Copy link
Contributor

@SamTheKorean SamTheKorean Nov 10, 2024

Choose a reason for hiding this comment

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

혹시 각각의 카드안에서 또 다시 github 아이콘과 기수 아이콘을 다른 section으로 나눈 이유가 있을까요? 이미 각각의 카드를 한 section으로 나누고 있는 구조에서 아이콘과 문자열을 위해서 section을 정의하는 건 불필요하게 복잡성을 증가시키는 것 같습니다! 단순히 배치를 조정하는게 목적이라면 div를 쓰는 것이 적절해보입니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<li>
  <img />
  <section> /* div로 바꾸기 */
  ....
  </section>
</li>

img, section 이구조가 목적에 안맞다고 코멘트를 이해했는데 맞을까요?
제가 이해한 부분이 맞다면 @SamTheKorean 님 의견에 동의합니다.

<li>
  <img />
  <section>  /* div로 바꾸기 */
     <section aria-labelledby={`card-github-${id}`}>  /* div로 바꿔야하는가?*/
      ...
     </section>
     <section aria-labelledby={`card-cohort-${id}`}>
      ...
     </section>
     ...
  </section>
</li>

추가로 궁금한점은 현재 내부에 section들도 변경을 해야하는지 여부인데요. 만약 코멘트의 의도가 맞으신다면 해당 부분도 div로 바꿔야할지 의문이네요. 뭔가 의미상으로 나눌수는 있는거 같아 section으로 작성했는데 다른 시멘틱태그가 있을까요? 이부분도 같은 의미로 코멘트로 남겨주신걸까요?

Copy link
Contributor

@SamTheKorean SamTheKorean Nov 11, 2024

Choose a reason for hiding this comment

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

네! 기존에 제가 의도한 건 section 안에 또 section을 만드는게 과한 것 같다는 의견이었습니다! 다만 저도 aside 컴포넌트를 작성하면서 section의 정의나 구분을 정확히 어떻게 나눠야할까 고민이 되는데 스스로도 명확한 답을 못내렸습니다..! 이 부분에 대해서 레퍼런스를 좀 더 찾아보겠습니다!

@SamTheKorean
Copy link
Contributor

SamTheKorean commented Nov 10, 2024

card component를 leaderboard에 적용해주시고 그에 대한 test도 작성해주셔야 컴포넌트를 완료했다고 볼 수 있을 것 같습니다! #45 (comment)

@Sunjae95
Copy link
Contributor Author

card component를 leaderboard에 적용해주시고 그에 대한 test도 작성해주셔야 컴포넌트를 완료했다고 볼 수 있을 것 같습니다! #45 (comment)

넵 리뷰감사합니다.
태그 부분 수정하고 cardcomponent 도입 후 수정하겠습니다 :)

@sounmind
Copy link
Member

sounmind commented Nov 21, 2024

image

테스트가 실패하고 있네요..?

@Sunjae95
Copy link
Contributor Author

image 테스트가 실패하고 있네요..?

맞아요. 이 부분이 아마도 2가지 방법으로 예상이 되는데요.

  1. github actions에서 token값이 없어서 데이터가 null이라서 카드컴포넌트가 존재하지않음
  2. useMembers에서 불러오는 훅과 테스트 예상값과 다름

1번은 토큰이 제대로 들어가 있는지 확인하려다 해결못했구요.
2번은 mocking을 하다가 꼬이는 바람에 시도를 못하고 있는 상황이네요.
공유했어야했는데 이번주 너무 바빠서 공유를 못드렸네요 죄송합니다🥲

Copy link
Contributor

@SamTheKorean SamTheKorean left a comment

Choose a reason for hiding this comment

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

대부분 완성이 되었고 아직 수정할게 남으셨겠지만 오늘 데모를 위해 미리 승인해두도록 하겠습니다!

src/components/Leaderboard/Leaderboard.tsx Outdated Show resolved Hide resolved
/>
</svg>
</div>
<span id={`card-cohort-${id}`}>{cohort}기</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

id 속성값은 하나의 페이지에서 유일해야하는데 44번째 줄과 이 줄의 id가 중복됩니다. (참고: https://dequeuniversity.com/rules/axe/4.10/landmark-unique)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제가 이해한 코멘트는 name이나 cohort가 중복이 가능하기에
image
이 경우에 해당된다는 말씀이실까요??

Copy link
Contributor

Choose a reason for hiding this comment

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

Shot 2024-11-23 at 17 35 59

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제가 이 코멘트의 의도를 파악하지 못한거 같아요.

section에 aria-labelledby를 지정한 이유는 section이 span의 text로 읽히는것을 의도로 작성했는데 사용법이 잘 못된걸까요?
이렇게 생각한 이유는 #17 (comment) 해당 코멘트보고 aria-labelledby를 사용하는것이 좋겠다라고 생각했습니다.
예시로 test code에서도 이름, 기수를 테스트하는 코드가 있습니다.

만약 이 방법이 id를 사용했기에 잘못된것이라면, 달레님 의도는 aria-labelledby과 id은 하나의 페이지에서 한번만 사용해야하며, 현재 card 컴포넌트는 자식요소이며, 중복되기에 aria-labelledby과 id를 사용하지말고 aria-label을 이용하여 이름, 기수 지정하라는 말씀일까요?

0370de6 해당 커밋에서 aria-label을 사용하여 코드를 올렸습니다.

Copy link
Contributor

@DaleSeo DaleSeo Nov 29, 2024

Choose a reason for hiding this comment

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

@Sunjae95 님, 뭔가 큰 오해가 있으신 것 같습니다. 제가 맨 처음에 말씀드린 것처럼 단순히 한 페이지에 동일한 id 속성값이 여러 개 있어서 문제가 됩니다. aria-labelledby 속성을 id와 연결하는 부분은 아무 상관이 없습니다.

우선 HTML의 id 속성 관련 관련 MDN 문서를 정독해보시기를 추천드리고요.

The id global attribute defines an identifier (ID) which must be unique in the whole document.

ID는 고유 식별자이므로 말 그대로 하나의 웹 페이지 내에서 유일해야합니다.

The purpose of the ID attribute is to identify a single element when linking (using a fragment identifier), scripting, or styling (with CSS).

브라우저에서 주소창에 https://leaderboard.dalestudy.com#card-cohort-daleseo를 입력하면 id 속성이 "card-cohort-daleseo"인 요소로 포커스가 이동합니다. 만약에 같은 id 속성값이 "card-cohort-daleseo"이 요소가 두 개라면 포커스가 첫 번째로 가야할까요 두 번째로 가야할까요?

카드 컴포넌트가 한 페이지에 여러 개 나타날 때도 id 중복이 생기지 않도록 신경을 써주셔야 합니다.

부연 설명이 도움이 되었으면 좋겠습니다! 🙏

Copy link
Member

@sounmind sounmind Nov 29, 2024

Choose a reason for hiding this comment

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

@DaleSeo 캡처하신 것을 보면 "cohort"와 "github"는 달라서 id가 중복될 일이 없어보이는데 저는 이 부분을 잘 이해 못했습니다ㅠ

  • card-cohort-daleseo
  • card-github-daleseo
image

브라우저에서도 해당 id를 가진 요소는 하나라고 나오는데 제가 어떻게 찾아보면 될까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sounmind 님, 수정된 구현에 문제가 있다는 것이 아니라 저는 단순히 선재님 질문에 답변을 드린 겁니다.

src/components/Card/Card.tsx Outdated Show resolved Hide resolved
src/components/Card/Card.test.tsx Outdated Show resolved Hide resolved
src/components/Card/Card.test.tsx Outdated Show resolved Hide resolved
@DaleSeo
Copy link
Contributor

DaleSeo commented Nov 29, 2024

@Sunjae95 UI Tests 체크 오른 쪽에 Details를 클릭하셔서 Chromatic에 들아가신 후 변경된 UI를 승인해주시기 바랍니다. (자세한 사용법은 다음 모임 때 설명드리겠습니다.)

Shot 2024-11-28 at 19 43 46

@Sunjae95 Sunjae95 merged commit caf289d into main Nov 30, 2024
4 checks passed
@Sunjae95 Sunjae95 deleted the 52-card-component branch November 30, 2024 02:34
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.

Card Component
4 participants