-
Notifications
You must be signed in to change notification settings - Fork 9
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
Issue164 리뷰 이미지 업로드 기능 추가 #165
The head ref may contain hidden characters: "issue164-\uB9AC\uBDF0-\uC774\uBBF8\uC9C0-\uC5C5\uB85C\uB4DC-\uAE30\uB2A5-\uCD94\uAC00"
Conversation
Co-Authored-By: Ashley Heo <51967731+ashleysyheo@users.noreply.github.com>
Co-Authored-By: Ashley Heo <51967731+ashleysyheo@users.noreply.github.com>
Co-Authored-By: Ashley Heo <51967731+ashleysyheo@users.noreply.github.com>
Co-Authored-By: Ashley Heo <51967731+ashleysyheo@users.noreply.github.com>
Co-Authored-By: Ashley Heo <51967731+ashleysyheo@users.noreply.github.com>
Co-Authored-By: Ashley Heo <51967731+ashleysyheo@users.noreply.github.com>
Co-Authored-By: Ashley Heo <51967731+ashleysyheo@users.noreply.github.com>
Co-Authored-By: Ashley Heo <51967731+ashleysyheo@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
코드에 남긴 리뷰 외에도 기획과 UI 상 질문이 있는데요,
- 이미지는 하나만 추가할 수 있던데 혹시 여러 개의 이미지를 추가하는 기능에 대해서는 계획이 없으신가요?? 만약에 계획은 있는데 지금은 구현이 안된 것이라면 imageUrl 타입을 배열로 받는 것은 어떨까 하는 생각도 했습니다.
- 이미지를 하나 밖에 업로드할 수 없지만 현재 UI 상으로는 여러 개 올릴 수 있는 것 같은 느낌이더라구요. 만약에 이미지를 수정하는 것을 가능하게 하기 위해서 현재 UI (버튼과 이미지가 같이 보이는 것)을 구성한거라면, 혹시 업로드한 이미지를 지우고 (x 버튼처럼), 다시 업로드 버튼을 보여주는 방식은 어떠신가요?
- 업로드 된 이미지의 스타일이 좀 심심한 것 같아요! 혹시 border-radius 정도는 넣을 생각은 안해보셨나요??
export const ReviewImage = styled.img` | ||
height: 10rem; | ||
object-fit: contain; | ||
object-position: left top; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object-position 옵션이 필요한 이유는 무엇인가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 ReviewContentWrapper
에서 display: flex
가 설정되어 있어서, 각 자식(?)들이 전체 width
를 차지하고 있습니다. 그래서 이미지의 width
가 자동으로 전체 너비를 차지하기 때문에 사진이 찌그러지는 문제가 발생해서 object-position
을 추가했습니다!
업로드 된 이미지의 스타일이 좀 심심한 것 같아요! 혹시 border-radius 정도는 넣을 생각은 안해보셨나요??
원래는 border-radius
를 추가하고 싶었습니다...!
그런데 object-fit
으로 비율을 조절하고 있기 때문에 img
의 실제 너비는 전체 width
가 되어서 border-radius
를 설정하면 왼쪽 위아래만 적용되는 문제가 발생해서 일단 뺐습니다. 이 부분을 어떻게 해결하면 좋을지 생각해 내지 못해서 일단 border-radius
를 뺐습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변경사항 확인하였습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
디렉토리명 > image보다 더 구체적으로 정하면 어떨까요?
review 디렉토리에 넣지 않고 image를 새로 만든 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이미지 api 자체를 재사용할 수 있게 만들기 위해서 리뷰를 보낼 때 이미지 파일 데이터를 보내서 imageUrl을 생성하는 것이 아닌 별도의 api를 만들었다고 백엔드한테 들었습니다. 그런 의미에서 추후에 이 api가 리뷰가 아닌 다른 상황에서 사용될 것을 생각하면 images를 만드는 것이 좀 더 어울리지 않나 싶어서 그렇게 했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 그러면 한 api로 모든 종류의 이미지 업로드를 다루는건가요?? 아니면 나중에 /image/review 식으로 세부적으로 나뉘나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 이해하기로는 이 images api로 모든 종류의 이미지 업로드를 다 다룬다고 알고 있습니다! 그래서 백엔드 쪽에서도 리뷰 폼을 제출할 때 이미지 파일 데이터를 보내는 것이 아니라, 일차적으로 이미지 url을 요청한 다음에 그 url을 리뷰 보낼 때 함께 보내도록 만든 것 같아요.
src/components/common/ImageUploadInput/ImageUploadInput.style.ts
Outdated
Show resolved
Hide resolved
엇 왜죠... 🥲 혹시 accessToken 여부를 검증하는 로직 때문에 오류가 나는 거 아닐까요....??
지금 위처럼 이미지 api에서도 accessToken을 보내야 해서 msw로 확인할 때
이것도 제 컴퓨터에서는 안 보여서 몰랐어요 🥲 일단, |
오 저는 두 문제 다 이제는 해결된 것 같네요! 수고하셨습니다! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&.uploaded { | ||
display: none; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스타일 적용하는 방식을 줄이는 게 좋을 것 같아요! 블링이 말한 함수 활용도 좋네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 그러면 한 api로 모든 종류의 이미지 업로드를 다루는건가요?? 아니면 나중에 /image/review 식으로 세부적으로 나뉘나요?
같은 이미지를 업로드할 생각을 못 해봤네요... ㅎㅎ 찾아보니까
아 이 얘기였군요!! ㅎㅎ |
Kudos, SonarCloud Quality Gate passed! |
#164
@ashleysyheo