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: Certificate page implement #18

Merged
merged 4 commits into from
Oct 28, 2024
Merged

Conversation

Sunjae95
Copy link
Contributor

@Sunjae95 Sunjae95 commented Oct 18, 2024

  1. @sounmind pdf 프린트하는 부분은 media print로 구현가능하다고 생각드는데 어떻게 생각하시나요? media print를 사용하지 않으면 html2canvas + jsPDF 라이브러리 생각중이긴합니다!
  2. linkedIn으로 넘겨줄때 field를 어디까지 정의해야하는지 명확하지않아 임의로 넣었습니다. 3주차 회의때 정하는건 어떨까요?
  3. [spike] 링크드인 수료증을 어떻게 연동할 수 있을까? #3 에서 예시 문서 밑에 아래와 같은 이미지가 존재하더라구요.
    ko_KR
    이런 버튼이 있는데 이것도 사용할지 아니면 디자인에 따라갈지 3주차 회의에서 정하는건 어떨까요?

@Sunjae95 Sunjae95 self-assigned this Oct 18, 2024
@Sunjae95 Sunjae95 linked an issue Oct 18, 2024 that may be closed by this pull request
Copy link
Contributor

@DaleSeo DaleSeo left a comment

Choose a reason for hiding this comment

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

@Sunjae95 작성하신 컴포넌트에 대한 스토리랑 테스트 파일도 추가해주실 수 있으실까요?

@sounmind 페이지 레벨 컴포넌트 파일을 src/ 바로 아래에 두시기를 의도하신 걸까요? 저는 아래와 같은 폴더 구조를 추천드리고 싶은데 어떻게 생각하시는지요?

├── eslint.config.js
├── index.html
├── package.json
├── public
│   └── vite.svg
├── src
│   ├── App.css
│   ├── App.tsx
│   ├── assets
│   │   └── react.svg
│   ├── components
│   │   └── Leaderboard
│   │       ├── Leaderboard.stories.ts
│   │       ├── Leaderboard.test.tsx
│   │       └── Leaderboard.tsx
│   │   └── Certificate
│   │       ├── Certificate.stories.ts
│   │       ├── Certificate.test.tsx
│   │       └── Certificate.tsx
│   ├── index.css
│   ├── main.tsx
│   ├── setupTests.tsx
│   └── vite-env.d.ts
├── tsconfig.json
└── vite.config.ts

@sounmind
Copy link
Member

sounmind commented Oct 18, 2024

@DaleSeo 폴더 구조 동의합니다. 컴포넌트 폴더 아래 관련 파일들이 다 모여있어서 보기 편할 것 같네요. 페이지 급의 컴포넌트는 3개 밖에 없으니 이 방식이 좋아보입니다.

@Sunjae95 media print는 처음 들어봤어요 👍 media print로 구현했을 때, html2canvas, jspdf와 비교한 장단점은 어떤 것인지 설명해주실 수 있을까요?

@Sunjae95
Copy link
Contributor Author

@DaleSeo 폴더 구조 동의합니다. 컴포넌트 폴더 아래 관련 파일들이 다 모여있어서 보기 편할 것 같네요. 페이지 급의 컴포넌트는 3개 밖에 없으니 이 방식이 좋아보입니다.

@Sunjae95 media print는 처음 들어봤어요 👍 media print로 구현했을 때, html2canvas, jspdf와 비교한 장단점은 어떤 것인지 설명해주실 수 있을까요?

제가 먼저 장단점을 설명해드렸으면 더 좋았을텐데 아쉽네요 😅

제가 생각하는 media print의 장점은 아래와 같습니다.

  1. 복잡한 view가 아닌 이상 css로 �처리할 수 있기에 별도의 라이브러리가 없어도 된다.
  2. html2canvas, jspdf는 동작하는데 비동기로 동작하지만 media print는 별도의 비동기 로직이 없다.
  3. 높이가 동적이지 않고 view layout의 변경이 없기에 적합

하지만 유의할 점도 존재하는데요. 이 부분은 html2canvas, jspdf에서도 동일하다고 생각돼요.

  1. 화면을 캡쳐하기 때문에 print view를 고려하여 css 속성 수정이 필요
  2. 브라우저별 동일하게 나오는지 확인

라이브러리를 추가하는 task이기에 저도 어떻게 해야할지 몰라 우선 media print를 사용하여 의견을 물어봤어요 🙂

@sounmind
Copy link
Member

@Sunjae95 상세한 설명 감사합니다 ☺️ 말씀을 들었을 때 media print 사용하는 것이 좋아보이는데요?
하지만 현재 이 티켓의 요구사항은 모든 기능의 구현이 아니고 디자인도 나오지 않아서 ... (이미 고민하셨겠지만!) 지금 상세 구현 코드까지는 작성하기는 어려울 것 같아요.
이 PR에서는 마크업이나 관련 함수들의 인터페이스 정도만 작성하는 것은 어떨까요?

@Sunjae95
Copy link
Contributor Author

@sounmind 맞아요 아직 정해지지 않았는데 과하게 생각한거 아닌가 싶네요😅
우선은 코멘트 대로 마크업이나 관련함수 인터페이스만 작성하고 추가로 달레님께서 story, test작성 요청해주셨는데 여기까지 마무리해서 올리겠습니다.🙂

src/components/Certificate/certificate.tsx Outdated Show resolved Hide resolved
src/components/Certificate/certificate.tsx Outdated Show resolved Hide resolved
src/components/Certificate/certificate.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@DaleSeo DaleSeo left a comment

Choose a reason for hiding this comment

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

@Sunjae95 님, 프런트엔드 테스트 코드를 참 읽기 편하게 잘 작성하시네요 👍

src/components/Certificate/certificate.test.tsx Outdated Show resolved Hide resolved
src/components/Certificate/certificate.test.tsx Outdated Show resolved Hide resolved
src/components/Certificate/certificate.test.tsx Outdated Show resolved Hide resolved
});

test("calls window.print when the print button is clicked", async () => {
vi.spyOn(window, "print").mockImplementation(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

다른 테스트에 불필요한 영향을 취소화하기 위해서 window.print()를 원상 복귀해주는 코드를 beforeAfter()에 추가해주셔야할 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

이 거 조치하신 것을 보았는데 제 피드백이 약간 잘못 전달이 된 것 같습니다.

모킹하는 부분은 여기에 그대로 두셔도 되고, 다음 코드가 추가되야 할 것 같습니다.

afterAll(() => {
  vi.mocked(window.print).mockRestore();
});

Copy link
Contributor

@DaleSeo DaleSeo 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

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

@Sunjae95 Sunjae95 Oct 26, 2024

Choose a reason for hiding this comment

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

파일명 변경하는 부분에서 시간이 걸렸네요.
문제가 되는 부분은 두가지였어요.

1. 깃은 파일, 폴더의 대소문자를 구분못한다.

해결방법

  • git config core.ignorecase false 로 대소문자 변경시 깃이 트래킹하도록 변경하는 옵션
  • git mv certificate Certificate 명령어를 통한 파일명 변경

2. 이름을 변경해도 이전 파일이 유령파일로 남아있어 로컬작업이 불가능함

예시)
image
해결방법
원격레포지토리에 1번에 변경된 내용을 push하고 로컬 프로젝트를 제거하고 다시 clone 받는다.
참고) 커밋 5a3c108 0d7b58d

src/components/Certificate/certificate.tsx Outdated Show resolved Hide resolved
src/components/Certificate/certificate.tsx Outdated Show resolved Hide resolved
src/components/Certificate/index.tsx Outdated Show resolved Hide resolved
rename: export name modify

fix: function keyword, div tag remove

fix: convert CSF 3.0

fix: test label

fix: local variable position

fix: reset mock window.print

fix: apply userEvent.setUp

fix: change method name from test to it

fix: mock reset

fix: aria-labelledby test

fire: Certificate/index.tsx

rename: file name temp 변경

rename: Certificate로 변경

fix: Certificate import 수정

fix: tag, useParams query

fix: change to test
@Sunjae95 Sunjae95 merged commit a6c9946 into main Oct 28, 2024
1 check passed
@Sunjae95 Sunjae95 deleted the 15-certificate-only-frame branch October 28, 2024 09:10
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 빼고)
3 participants