ImageUpload 컴포넌트 만들기 #334

NamJwong · 2022-05-22T22:03:26Z

⛓ Related Issues

close 이미지 수정/삭제 핸들 공통 컴포넌트 만들고 팀/이슈 등록에 적용 #324

📋 작업 내용

ImageUpload 컴포넌트 만들기
이슈 수정하기에 적용

📌 PR Point

ImageUpload는 FileUpload를 수정해서 만들었고 useImageUpload 훅이랑 바텀시트랑 같이 써야 합니다
컴포넌트와 훅을 합치지 못한 이유는 바텀시트의 onClick 이벤트에 관련 상태를 써야 하기 때문에 ~ ~
바텀 시트는 기본 형태에 useImageUpload에서 주는 버튼 리스트만 넣어주면 됩니다
이것은 팀 수정하기에 적용 안시켜서 그러는건데 리뷰 반영 완료하고 나서 팀 수정하기 포함해서 이미지 파일 핸들하는 곳들 모두 적용시키고 머지하겠습니다~!

SeojinSeojin · 2022-05-22T22:03:42Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

SeojinSeojin

수과셨어용!! 요즘 훅 세상에서 날라다니는 것 같아서 좋아용 ㅎㅎ

모든 이미지 적용되는 컴포넌트에 이 친구를 쓸 필요성은 크지 않게 느껴져용 .!! 지금 성능에 문제 있거나 모두가 바텀시트 쓰는게 아니니까 FileInput으로 하는 모든 부분을 바꿀 필요는 없어용~ 바텀시트 써야되는 친구들만 바꿔주시면 될 것 같아용
Image는 File, Thumbnail은 url string인 원칙을 굳이 깰 필요는 없을 것 같아용 .. 디폴트 썸네일 값을 프롭으로 주고 그걸 thumbnail 상태의 기본값으로 갖게 하는 것이 좋을 것 같습니당~

SeojinSeojin · 2022-05-22T23:17:35Z

src/presentation/components/common/ImageUpload/index.tsx

+import { StImageUpload, StThumbnail, StUploadButton } from './style';
+
+interface ImageUploadProps {
+  image: File | string | null;


String은 왜 가능해용?

밑에서 깨달았는데 File이랑 null만 하고, 디폴트 이미지가 있는 경우에는 그걸 프롭으로 받아서 thumbnail 상태의 기본 값으로 쓰면 좋을 것 같아용~

관련 내용 하나에 답변 달겠습니다~!

SeojinSeojin · 2022-05-22T23:19:50Z

src/presentation/components/common/ImageUpload/index.tsx

+        ref={ref}
+        type="file"
+        onChange={handleFileInput}
+        accept="image/jpeg, image/png, image/gif"


여기서 이미지 아닌 파일은 선택 못 하게 막아줘서 정규식으로 imageFileForm 체크하는 부분은 필요 없을 것 같아요!!

사실 저도 있어야 하나 궁금했던 부분인데 알려주셔서 감사합니다~! 뺄게여!!!

SeojinSeojin · 2022-05-22T23:21:50Z

src/presentation/components/common/ImageUpload/index.tsx

+  image: File | string | null;
+  style: {
+    width: string;
+    height: string;


Number도 받아주면 좋겠네용~

string이랑 number 둘다 되게요?? 혹시 number 받는게 더 나으면 number로 바꿀게요!

SeojinSeojin · 2022-05-22T23:23:48Z

src/presentation/components/common/ImageUpload/index.tsx

+  const { fireToast } = useToast();
+
+  const clickUploadButton = () => {
+    if (!ref) return;


요건 왜 처리된 거에요?

이것은 삽질 과정의 잔재입니다! 삭제할게요!👍

SeojinSeojin · 2022-05-22T23:25:41Z

src/presentation/components/common/ImageUpload/index.tsx

+  const [imageThumbnail, setImageThumbnail] = useState('');
+  const { fireToast } = useToast();
+
+  const clickUploadButton = () => {


이름을 보면 업로드버튼에 붙여있을 것 같은데 업로드 전체에 붙어있네용~
clickImageUpload는 어때용?

줳습니다!!!!

SeojinSeojin · 2022-05-22T23:28:07Z

src/presentation/components/common/ImageUpload/index.tsx

+        {image ? (
+          <StThumbnail
+            src={typeof image === 'string' ? image : imageThumbnail}
+            width={width}


아무 가공 안 하고 값이 바로 입력되는 거면 스컴포넌트까지 가지 않아도 될 것 같아용~
style 속성에 때려박아도 될 것 같아요~

때려박아볼게요!

저는 갠적으로 인라인 스타일 지양하는 편이라 주영언니의 이전 코드도 좋았습니다!
그치만 서진이가 이렇게 제안한 이유도 이해가 되기 때문에.. 둘 다 좋다는 의견 남기고 싶었습니다 ☺️

https://code-masterjung.tistory.com/68

리액트에서의 인라인 스타일 관련 글인데 공유하면 좋을 것 같아서 링크 하나 남깁니다‼️

허고곡 이런 이슈가 있군요 넘 감사합니다!!!! 이 부분은 단순 width, height, borderRadius 정도니까 두고 앞으로 지양하겠슴다!!

이것은 저 글 다시 읽어보니 빼면 좋을 것 같아서 스컴포넌트로 다시 옮길게요

SeojinSeojin · 2022-05-22T23:30:29Z

src/presentation/pages/Team/Issue/Edit/index.tsx

@@ -44,7 +43,8 @@ export default function TeamIssueEdit() {
    () => api.teamService.getIssueInfo(issueID ?? ''),
    {
      onSuccess: () => {
-        id !== issueInfo?.writerID && navigate('/');
+        if (issueInfo && id === issueInfo.writerID) setImage(issueInfo.team.thumbnail);


Image 말고 Thumbnail을 set해주면 안돼용? 괜히 타입이 꼬이는 것 같아용 ..

defaultThumbnail이라는 프롭 하나 만들어서 넘기면 좋을 것 같아용!

관련 내용 하나에 답변 달겠습니다~!

SeojinSeojin · 2022-05-22T23:34:07Z

src/presentation/pages/Team/Issue/Edit/index.tsx

@@ -92,7 +90,7 @@ export default function TeamIssueEdit() {
          selectedCategory.id,
          issueTextarea,
          image,
-          image ? 'NEW' : issueInfo?.team.thumbnail && isImageDeleted ? 'DELETE' : 'NONE',
+          typeof image === 'string' ? 'NONE' : image ? 'NEW' : 'DELETE',


옛날 비교 방식이 더 좋은 것 같아용 ..
3주 후에 다시 봤을 때 왜 type이 string이면 none이지? 를 알아내는 데에 시간 꽤 걸릴 것 같습니다 ..
그리고 이미지 파일 타입 3개 되는 것도 좋아 보이지는 않아용 ..

일단 제가 이렇게 타입을 혼용한 이유는 어쨌든 이미지는 무조건 셋 중에 하나인 상태일테니까 이게 더 상태를 파악하기 쉽지 않나? 생각했었습니다 위에서 말씀하신 것처럼 thumbnail 상태를 다시 따로 두고 타입을 string, File | null로 각각 분리하면 thumnail도 값이 있고 imageFile도 값이 있을 때 imageFile을 최신 이미지라 판단하고, 이미지를 삭제할 때 imageFile과 thumbnail를 둘다 비워줘야 한다는게 오히려 복잡하게 느껴졌습니다

그른데

3주 후에 다시 봤을 때 왜 type이 string이면 none이지? 를 알아내는 데에 시간 꽤 걸릴 것 같습니다 ..

이 말씀이 정말 그럴 수 있을 것 같고 또 코드 속사정을 모르는 사람들이 봤을 때 절대 이해 못할 것 같아서 고민되네요..................

imageState?Status?('NONE' | 'NEW' ...) 이걸 상태로 따로 관리하면서 활용하면 그렇게 나눠도 깔끔해질 수도 있을것 같네요??? 어차피 수정 요청할 때 꼭 필요한 값이니까 상태가 하나 추가되는게 크게 나쁘진 않은 것 같아요 일단 그렇게 해보겠습니다! 그릏지만 이 방법도 우려되는 바가 있다면 말해주시면 감사하겠슴다!

NamJwong · 2022-05-23T06:38:39Z

모든 이미지 적용되는 컴포넌트에 이 친구를 쓸 필요성은 크지 않게 느껴져용 .!! 지금 성능에 문제 있거나 모두가 바텀시트 쓰는게 아니니까 FileInput으로 하는 모든 부분을 바꿀 필요는 없어용~ 바텀시트 써야되는 친구들만 바꿔주시면 될 것 같아용

모든 이미지는 등록하는 과정에서 삭제할수도 있어야 하지 않을까요?? === 모두 바텀시트 있어야 하지 않을까요?

SeojinSeojin · 2022-05-23T08:48:13Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

SeojinSeojin · 2022-05-23T09:37:01Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

100Gyeon

여기 코드 리뷰 핫해서 정독하는데 시간이 조금 걸렸네요,,
수정사항 반영한 부분도 멋있어요! 수고하셨습니다 ❤️‍🔥

100Gyeon · 2022-05-23T17:19:22Z

src/presentation/components/common/ImageUpload/index.tsx

+      if (checkBrowser('Internet Explorer')) {
+        setImage(file);
+        setImageThumbnail(URL.createObjectURL(file));


6월 15일부터 IE 지원 종료되니까 빼도 좋을 것 같아요 !! (반영해도 되고 안 해도 되는 작은 의견입니다..)
그래도 이런 세심함 너무 좋습니다 👍

울이 서진이가 FileUpload에 해놨던 코드 가져왔지유👍 빼도 되겠다!!!!

SeojinSeojin · 2022-05-24T01:17:44Z

나의 PR이 머지된걸 pull받고 머지하면 ..
St뭐시기가 없다고 하면 ..
ServiceCenter/index.tsx 의 파일 업로드 컴포넌트도 이것으로 교체해줘~!!!!!

SeojinSeojin · 2022-05-24T08:20:59Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

SeojinSeojin · 2022-05-24T08:25:14Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

100Gyeon

슬랙 보고 또 다녀갑니다 ☺️ !!

100Gyeon · 2022-05-24T10:54:14Z

src/application/hooks/useImageUpload.ts

-  const [image, setImage] = useState<File | null>(null);
+  const [image, setImage] = useState<File | null | undefined>();


여기 undefined가 추가된 이유가 무엇인가용?

이미지를 등록했다가 삭제한 경우에는 null이고 아예 새 이미지를 한번도 등록하지 않은 상태가 undefined입니다!

100Gyeon · 2022-05-24T11:03:59Z

src/presentation/components/common/ImageUpload/index.tsx

+
+  const handleFileInput = async (e: React.ChangeEvent<HTMLInputElement>) => {
+    e.preventDefault();
+    if (e.target.files !== null && e.target.files.length > 0) {


Suggested change

if (e.target.files !== null && e.target.files.length > 0) {

if (e.target.files?.length > 0) {

이렇게 수정하는 게 좋을 것 같습니다 !

넵 감사합니다!!

아이고 타입내로잉 해줬던거였어요 e.target.files !== null 이렇게 명시를 해야 밑에서 e.target.files를 쓸 수 있어요 HTMLInputElement.files: FileList | null 이게 e.target.files의 타입입니다!
그래서 그대로 두겠습니다!

SeojinSeojin · 2022-05-25T16:37:41Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

SeojinSeojin · 2022-05-25T16:39:08Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

NamJwong · 2022-05-25T16:39:16Z

나의 PR이 머지된걸 pull받고 머지하면 .. St뭐시기가 없다고 하면 .. ServiceCenter/index.tsx 의 파일 업로드 컴포넌트도 이것으로 교체해줘~!!!!!

서진쓰 상개발자라 충돌 안났어요 일단 머지하고 다른 부분에는 한 이슈에서 한번에 적용하겠습니다!

SeojinSeojin · 2022-05-25T16:43:45Z

충돌 나는거 말하는게 아니었어용 ..
이미지를 업로드해주세요~ 하는 부분의 스타일드컴포넌트가 사라져서 머지 컨플릭트는 없더라도 코드 내부적으로 오류가 날텐데 그 부분을 수정해달라는 의미였어요!

SeojinSeojin · 2022-05-25T18:16:44Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

SeojinSeojin · 2022-05-25T18:22:21Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

SeojinSeojin

아주 훌륭합니다!!

SeojinSeojin · 2022-05-26T00:48:32Z

지금 dev를 머지하면 yarn start 했을 때 오류가 나는데, 그 부분 수정하고 머지해주셔용₩!

SeojinSeojin · 2022-05-26T11:34:52Z

울 웹쁜이 고생많았어 ! 여기서 미리보기로 보면서 쉬어~ 다른 웹쁜이들한테도 자랑해줘~

✨ feat: ImageUpload 컴포넌트 만들고 이슈 수정하기에 적용

ce00e14

NamJwong added the feature 🎄 기능 개발 label May 22, 2022

NamJwong added this to the 5월 3주차 스프린트 milestone May 22, 2022

NamJwong requested review from SeojinSeojin and 100Gyeon May 22, 2022 22:03

NamJwong self-assigned this May 22, 2022

SeojinSeojin requested changes May 22, 2022

View reviewed changes

NamJwong added 4 commits May 23, 2022 17:44

📝 chore: 정규표현식으로 이미지 파일인지 검사해주는 로직 없애기

1d1ef51

📝 chore: 필요 없는 조건문 삭제

e85cb7a

🔨 refactor: 변수명 변경

7da5a5a

🔨 refactor: 썸네일 스타일 설정 위치 변경

859edb3

🔨 refactor: 스타일 적용 방식 변경

9586eb8

100Gyeon approved these changes May 23, 2022

View reviewed changes

NamJwong added 2 commits May 24, 2022 17:15

🔨 refactor: 이미지 파일과 썸네일 상태 분리

2e41d09

🔨 refactor: ImageUpload 컴포넌트에 필요한 prop 묶어주기

00acd8e

📝 chore: IE 관련 코드 및 console.log 삭제

093d0eb

100Gyeon approved these changes May 24, 2022

View reviewed changes

100Gyeon reviewed May 24, 2022

View reviewed changes

📝 chore: dev 브랜치 머지

9ea64d1

📝 chore: ux writing 수정 사항 반영

da8cc64

NamJwong added 2 commits May 26, 2022 03:00

🔨 refactor: 스타일 적용 방식 변경

e3c1462

✨ feat: 팀 수정하기에 ImageUpload 적용

4b93763

🎨 design: 스타일 컴포넌트 구조 변경

420ef47

SeojinSeojin approved these changes May 26, 2022

View reviewed changes

📝 chore: 문의하기 페이지에 ImageUpload 적용

a01d266

NamJwong merged commit 1b976b6 into dev May 26, 2022

NamJwong deleted the feat/#324 branch May 26, 2022 11:36

NamJwong mentioned this pull request Jun 7, 2022

QA 반영 #352

Merged

3 tasks

		const [image, setImage] = useState<File \| null>(null);
		const [image, setImage] = useState<File \| null \| undefined>();

	if (e.target.files !== null && e.target.files.length > 0) {
	if (e.target.files?.length > 0) {

ImageUpload 컴포넌트 만들기 #334

ImageUpload 컴포넌트 만들기 #334

Conversation

NamJwong commented May 22, 2022 • edited

⛓ Related Issues

📋 작업 내용

📌 PR Point

SeojinSeojin commented May 22, 2022

SeojinSeojin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamJwong May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamJwong May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamJwong May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamJwong May 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamJwong commented May 23, 2022

SeojinSeojin commented May 23, 2022

SeojinSeojin commented May 23, 2022

100Gyeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamJwong May 24, 2022 • edited

Choose a reason for hiding this comment

SeojinSeojin commented May 24, 2022

SeojinSeojin commented May 24, 2022

SeojinSeojin commented May 24, 2022

100Gyeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NamJwong May 25, 2022 • edited

Choose a reason for hiding this comment

SeojinSeojin commented May 25, 2022

SeojinSeojin commented May 25, 2022

NamJwong commented May 25, 2022

SeojinSeojin commented May 25, 2022

SeojinSeojin commented May 25, 2022

SeojinSeojin commented May 25, 2022

SeojinSeojin left a comment

Choose a reason for hiding this comment

SeojinSeojin commented May 26, 2022

SeojinSeojin commented May 26, 2022

NamJwong commented May 22, 2022 •

edited

NamJwong May 23, 2022 •

edited

NamJwong May 23, 2022 •

edited

NamJwong May 23, 2022 •

edited

NamJwong May 23, 2022 •

edited

NamJwong May 24, 2022 •

edited

NamJwong May 25, 2022 •

edited