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 : implement leaderboard component #17

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

SamTheKorean
Copy link
Contributor

No description provided.

@SamTheKorean
Copy link
Contributor Author

SamTheKorean commented Oct 18, 2024

@sounmind 테크 리드님! 질문있습니다. 에반님이 미리 names 배열을 만들어주셔서 data와 css도 추가해서 대강 만들고 있는데, 티켓 내용처럼 css나 data는 일절 넣지 않고 html만 이용해서 프레임을 만드는게 좋을까요?

@SamTheKorean SamTheKorean self-assigned this Oct 18, 2024
@SamTheKorean
Copy link
Contributor Author

요건 선재님 pr참고해서 바꿔보겠습니다.

@SamTheKorean SamTheKorean marked this pull request as ready for review October 18, 2024 20:15
src/main.tsx Outdated
@@ -14,11 +14,11 @@ const router = createBrowserRouter([
errorElement: <ErrorPage />,
},
{
path: "/members/:username/progress",
path: "/members/progress",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

달레님과 초기 셋업 pr에서 나눈 대화를 보고 바꿔봤습니다.

Copy link
Contributor

@sounmind sounmind Oct 19, 2024

Choose a reason for hiding this comment

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

PR의 이 커밋 반영할 예정이라, 이거 먼저 봐주시겠어요?!

@sounmind
Copy link
Contributor

테크 리드님! 질문있습니다. 에반님이 미리 names 배열을 만들어주셔서 data와 css도 추가해서 대강 만들고 있는데, 티켓 내용처럼 css나 data는 일절 넣지 않고 html만 이용해서 프레임을 만드는게 좋을까요?

넵 아직 디자인이 나온 것이 아니기 때문에 아주 간단하게만 만들어주시면 될 것 같습니다.
확실한 요구사항만 반영해주세요!

@SamTheKorean
Copy link
Contributor Author

amend 후 force push해서 commit을 정리하고 싶었는데 amend가 안되는 오류가 생겨서 해결해보다가 시간관계로 우선은 올렸습니다..

@sounmind
Copy link
Contributor

amend 후 force push해서 commit을 정리하고 싶었는데 amend가 안되는 오류가 생겨서 해결해보다가 시간관계로 우선은 올렸습니다..

오잉 무슨 오류가 생겼을까요?

@SamTheKorean
Copy link
Contributor Author

지워도 계속 생성되네요...
Screenshot 2024-10-18 at 4 18 39 PM

@SamTheKorean
Copy link
Contributor Author

따로 열린 터미널도 없고 COMMIT_EDITMS 저 파일도 계속 지워봤는데 지속적으로 생성되네요. 처음보는 오류입니다. 재부팅은 안해봤습니다.

@DaleSeo DaleSeo linked an issue Oct 20, 2024 that may be closed by this pull request
@SamTheKorean SamTheKorean marked this pull request as draft October 20, 2024 20:19
@SamTheKorean SamTheKorean reopened this Oct 20, 2024
@SamTheKorean SamTheKorean marked this pull request as ready for review October 21, 2024 01:22
@SamTheKorean SamTheKorean requested a review from a team as a code owner October 21, 2024 01:22
@SamTheKorean SamTheKorean marked this pull request as draft October 21, 2024 01:33
@SamTheKorean SamTheKorean changed the title feat : set up the rough frame for leaderboard component feat : implement leaderboard component Oct 21, 2024
@SamTheKorean SamTheKorean marked this pull request as ready for review October 21, 2024 01:39
Copy link
Contributor

@sounmind sounmind left a comment

Choose a reason for hiding this comment

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

이후에 실제 데이터를 불러올 수 있는 기능이 리더보드 컴포넌트에 적용된다면, 컴포넌트도 테스트도 높은 확률로 바뀌어야 하겠네요. 일단 뼈대를 잡는다는 것에 의미가 있겠습니다! 고생하셨습니다 🙇‍♂️

Copy link
Contributor

@Sunjae95 Sunjae95 left a comment

Choose a reason for hiding this comment

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

데이터까지 고려하여 queryclient까지 적용하셨군요
수고하셨습니다 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

지난 모임에서 폴더 구조 논의할 때 index.tsx 파일을 쓰지 않기로 하지 않았던가요?

Leaderboard
</h1>
<main>
<h1>Leaderboard</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

<main> 안에 <h1>을 넣는 것은 일반적인 HTML 구조에서 벗어나는 것 같습니다.

보통 스크린 리더 사용자를 위해서 아래와 같이 많이 구성합니다.

<header>
  <h1>Leaderboard</h1>
</header>
<main>
  <!-- 메인 컨텐츠 -->
</main>
<footer>
  <!-- 풋터 -->
</footer>

Copy link
Contributor

@DaleSeo DaleSeo Oct 25, 2024

Choose a reason for hiding this comment

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

전체 레이아웃과 헤더를 담당해줄 컴포넌트가 필요해보이네요. 이번 모임 안건으로 넣겠습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

예시 제공과 설명 감사합니다! 피드백 반영해서 고쳐보겠습니다!

Copy link
Contributor Author

@SamTheKorean SamTheKorean Oct 26, 2024

Choose a reason for hiding this comment

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

html 구조 같은 경우에는 main에서 div로 바꿨습니다! 다만 피드백을 듣고 공부하는 과정에서 궁금증이 생겨서 질문드립니다.

h1이 main에 들어는게 일반적이지 않다는 것은 어떤 맥락에서 말씀하신 건지 완전하게 이해가 안됩니다! w3schools 과 mdn 모두 main을 사용하는 예시에서는 h1이 main안에 있고, 직관적으로도 메인 컨텐츠안에 h1이 있는 것 자체만으로 이상하다고 느껴지지는 않아서요. 관련해서 검색해봐도 납득이 되는 레퍼런스가 없어서 혹시 제가 어떤 부분을 놓치고 있는지 궁금해서 질문드립니다! https://www.w3schools.com/tags/tag_main.asp, https://developer.mozilla.org/en-US/docs/Web/HTML/Element/main

Copy link
Contributor

Choose a reason for hiding this comment

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

간단한 웹페이지라면 <h1><main> 안에 넣어도 무방합니다. 하지만 그렇게 했을 때 문제점은 시간이 지남에 따라 <main> 영역이 점점 넓어져, 나중에는 웹사이트 전체가 <main> 안에 들어가버리는 현상이 발생합니다. 즉, 메인 컨텐츠를 구분해주는 본연의 역할을 상실하는 것이죠.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아하 친절한 설명 감사드립니다! main의 의미가 퇴색될 수 있다는 점 잘 이해가 갔습니다! 리더보드가 전체를 감싸고 있어서 더 문제가 될 수도 있다는 생각이 드네요 ㅎㅎ 감사합니다.

Comment on lines +15 to +16
<section aria-labelledby="leaderboard">
<h2 id="leaderboard">Members List</h2>
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 Author

Choose a reason for hiding this comment

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

달레님한테 배운걸로 기억합니다! 1기 웹사이트 만들 때 접근성 이유로 aria-label을 쓰라고 권장 받아서 이번에 기억나서 mdn 찾아서 썼는데 혹시 쓰임이 문제가 있을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

아니, 제가 이런 거까지 벌써 알려드렸던가요? ㅎㅎ 주니어 개발자가 aria-labelledby 이렇게 쓰는 거는 보기 드물어서 여쭤봤네요. 잘 하셨어요! 👏

딱 한가지 아쉬운 점은 이렇게 잘 lableling해놓으시고 정작 테스트에서 활용하시지 않으셨다는 거에요. 다음과 같은 테스트를 추가하시면 더 멋질 것 같습니다. 😁

test("renders the members list section", () => {
  expect(
    screen.getByRole("region", { name: /members list/i }),
  ).toBeInTheDocument();
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

피드백 감사합니다 ㅎㅎ 네네! 달레님 덕분에 접근성을 고려하는 법을 여러 방면으로 배웠습니다! 테스트 피드백도 현재 어느정도로 하면 좋을 지 확신이 안썼는데 예시까지 감사드립니다! 반영해보겠습니다!.

Comment on lines +2 to +8
const members = [
{ name: "DaleSeo", solved: 71, rank: "새싹" },
{ name: "sounmind", solved: 69, rank: "나무" },
{ name: "yolophg", solved: 65, rank: "새싹" },
{ name: "Sunjae95", solved: 63, rank: "나무" },
{ name: "HC-kang", solved: 62, rank: "나무" },
{ name: "SamTheKorean", solved: 60, rank: "나무" },
Copy link
Contributor

Choose a reason for hiding this comment

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

타이핑이 안 되어 있어서 좀 아쉽네요. 흠... 일단 두시죠. 데이터를 제공하는 모듈에서 타입을 제공하면 될 것 같습니다.

<div>푼 문제: {member.solved}</div>
<div>
<a href={`/progress?member=${member.name}`}>
<button>풀이 보기</button>
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 Author

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.

#22 (comment) 에 👍 하셨으니 폴더명과 파일명도 대문자로 시작하도록 고치면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 고치겠습니다!

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.

리더보드 페이지 구현 (CSS 빼고, Data 빼고)
4 participants