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] Firebase image 업로드 할 수 있도록 컴포넌트와 훅 추가 #107

Merged
merged 13 commits into from
Oct 10, 2021

Conversation

YuuRiLee
Copy link
Contributor

@YuuRiLee YuuRiLee commented Sep 17, 2021

PR 제안 사유

Resolve #102

주요 변경 기록

  • 이미지를 업로드 할 수 있는 FileUploader 컴포넌트를 추가합니다
  • 파이어베이스 초기화를 해주는 firebaseInit 파일을 추가합니다
  • firebase storage에 업로드하는 hook을 추가합니다

Code review

Code review 에서 중점적으로 봐야하는 부분

Design review

Design review 에서 중점적으로 봐야하는 부분 / 스크린샷

기타 질문 및 특이 사항

@YuuRiLee YuuRiLee added the Feat 새로운 기능과 관련된 작업 label Sep 17, 2021
@YuuRiLee YuuRiLee self-assigned this Sep 17, 2021
src/components/FileUploader/index.tsx Outdated Show resolved Hide resolved
src/components/FileUploader/index.tsx Outdated Show resolved Hide resolved
src/components/FileUploader/index.tsx Outdated Show resolved Hide resolved
src/components/FileUploader/index.tsx Outdated Show resolved Hide resolved
src/pages/CreateDesign/Detail/index.tsx Show resolved Hide resolved
src/hooks/useFirebaseStorage.ts Outdated Show resolved Hide resolved
src/hooks/useFirebaseStorage.ts Outdated Show resolved Hide resolved
src/hooks/useFirebaseStorage.ts Outdated Show resolved Hide resolved
Base automatically changed from design-input-change to sprint-2-1 October 10, 2021 09:29
@YuuRiLee
Copy link
Contributor Author

@zoripong 해당 커밋 부터 봐주세요

Copy link
Member

@zoripong zoripong 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/FileUploader/index.tsx Outdated Show resolved Hide resolved
src/components/FileUploader/index.tsx Outdated Show resolved Hide resolved
src/components/FileUploader/FileUploader.css.ts Outdated Show resolved Hide resolved
src/hooks/useFirebaseStorage.ts Outdated Show resolved Hide resolved
src/hooks/useFirebaseStorage.ts Outdated Show resolved Hide resolved
src/hooks/useFirebaseStorage.ts Outdated Show resolved Hide resolved
src/hooks/useFirebaseStorage.ts Outdated Show resolved Hide resolved
@zoripong zoripong self-requested a review October 10, 2021 11:51
Copy link
Member

@zoripong zoripong left a comment

Choose a reason for hiding this comment

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

어푸르브 아니고 코멘트..

@YuuRiLee
Copy link
Contributor Author

@zoripong 해당 커밋부터 + 코멘트 봐주세요

Copy link
Member

@zoripong zoripong left a comment

Choose a reason for hiding this comment

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

saso 한 하나만 찐 막으로 확인해쥬세용

@@ -28,11 +27,9 @@ type FirebaseStorage = Omit<UploadStorage, 'error'> & {
};

const useFirebaseStorage = (path: string): FirebaseStorage => {
const history = useHistory();
const token = getAccessToken();

if (token == null) {
Copy link
Member

Choose a reason for hiding this comment

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

p3: 이 체크를 아예 안해도 되지 않을까요? 이제 여기가 true가 될 일이 없을 것 같은뎅 👀

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
Member

Choose a reason for hiding this comment

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

아 안돼....

Copy link
Member

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ 빈문자열이 더 싫은데... undefined 반환은 어떠신가요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

undefined로 리턴하면 지금처럼 if (token == null) 이렇게 체크해줘야해욤,,

Copy link
Member

Choose a reason for hiding this comment

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

그냥 체크하기로 합시댜....... ㅠ_ㅠ
리턴타입에 void 섞여있어서 그런거죠?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

redirectToLogin 리턴 타입을 void로 해서 그래용 undefined로 바꿔도 마찬가지기는 합니드앙

Copy link
Member

@zoripong zoripong left a comment

Choose a reason for hiding this comment

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

ㅅㅜㄱㅗ하셨습니당

@YuuRiLee
Copy link
Contributor Author

@jiyaaany 나중에 여기 커밋들 확인해주세용

@YuuRiLee YuuRiLee merged commit 042232d into sprint-2-1 Oct 10, 2021
@YuuRiLee YuuRiLee deleted the firebase-image branch October 10, 2021 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feat 새로운 기능과 관련된 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants