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

이미지 다운로드 모바일 기기 대응 #424

Merged
merged 5 commits into from
Jul 23, 2023
Merged

Conversation

sumi-0011
Copy link
Member

🤔 해결하려는 문제가 무엇인가요?

아이폰 사파리 외에 다른 모바일에서 이미지 다운로드가 되지 않는 문제가 있었어요

🎉 변경 사항

  • 이미지 다운로드 로직 추가 (분기)
  • navigator.share기능이 지원되는 디바이스라면, 공유를 하도록 하고,
  • 아이폰 사파리이거나 데스크탑이면 a 태그를 이용해 다운 받을 수 있도록 하였습니다.
  • 안드로이드 등의 브라우저를 대응하기 위해 모달을 띄우고 꾹 눌러서 다운 받을 수 있도록 했어요.

@sumi-0011 sumi-0011 changed the title Fix/image down 이미지 다운로드 모바일 기기 대응 Jul 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (1d81ce0) 91.71% compared to head (cca8817) 91.71%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #424   +/-   ##
=======================================
  Coverage   91.71%   91.71%           
=======================================
  Files          44       44           
  Lines         326      326           
  Branches       55       55           
=======================================
  Hits          299      299           
  Misses         27       27           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Jul 22, 2023

Bundle Sizes

Compared against 1d81ce0

Route Size (gzipped) Diff
/dna/[id] 102.97 KB +1.92 KB
/result/test no change removed

Dynamic import: No significant changes found

@sjoleee
Copy link
Member

sjoleee commented Jul 23, 2023

lgtm

Comment on lines 1 to 2
// 사용 방법
// detectMobileDevice(window.navigator.userAgent)
Copy link
Member

Choose a reason for hiding this comment

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

이 부분도 tsdoc으로 써봐도 조흘거가타요~~

Copy link
Member Author

Choose a reason for hiding this comment

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

좋습니당!

Comment on lines 79 to 90
// 1. 공유 모드가 가능하다면 공유 모드로 공유
if (typeof navigator.share !== 'undefined') {
if (await imageShare(imageBase64)) return;
}

// 2. desktop 이거나 safari이면 다운로드
if (!detectMobileDevice() || browser === 'Safari') {
imageDownloadPC(imageBase64, 'dna');

return;
}
// 3. mobile이면 꾹 눌러서 다운로드
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 Author

Choose a reason for hiding this comment

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

오 그렇다면 주석 제거하겠습니다!
주석 없어도 알아볼수 있다니 성공했다 🚀

Comment on lines 80 to 82
if (typeof navigator.share !== 'undefined') {
if (await imageShare(imageBase64)) return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (typeof navigator.share !== 'undefined') {
if (await imageShare(imageBase64)) return;
}
if (typeof navigator.share !== 'undefined') {
await imageShare(imageBase64)
return;
}

이렇게 해도 되는 거 같은데, 표현식을 if 문으로 감싸주신 이유가 있으신가요??

Copy link
Member Author

Choose a reason for hiding this comment

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

imageShare 함수를 확인해보면 리턴값이 boolean인데, 혹시 실패하는 상황에는 return 하지 않음으로써
후순위에 있는 이미지 다운 기능을 실행하기 위해 이렇게 작성하였습니다!

더 좋은 방향을 아신다면 알려주세요!

Copy link
Member

@hyesungoh hyesungoh Jul 23, 2023

Choose a reason for hiding this comment

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

오호 이미지 쉐어가 실패할 수도 있군뇨 몰랐어용 👍 👍

const isImageShared = await imageShare(imageBase64);

if (isImageShare) return;

이렇게 풀어 쓸 수도 있을 거 같은데, 지금도 좋아 보입니다 ~~

Copy link
Member Author

Choose a reason for hiding this comment

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

좋습니당~

@sumi-0011 sumi-0011 merged commit 9a4fc6d into main Jul 23, 2023
9 checks passed
@sumi-0011 sumi-0011 deleted the fix/image-down branch July 23, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

이미지 다운 모달 뱃지 스타일 수정 이미지 다운 모달 페이지에 연결
4 participants