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

[Feature] - 프로필 이미지 수정 기능 및 여행기 등록 시 여행 장소마다 국가 코드 주도록 구현 #535

Open
wants to merge 22 commits into
base: develop/fe
Choose a base branch
from

Conversation

simorimi
Copy link

✅ 작업 내용

  • 프로필 이미지 수정 기능 구현
  • 여행기 등록시 여행 장소마다 국가 코드 주도록 구현
  • drawer 헤더 부분 기존의 헤더와 크기 다르던 부분 수정

📸 스크린샷

2024-10-16.10.56.55.mov

🙈 참고 사항

프로필 이미지 수정 기능 웹에서는 정상 동작합니다. 다만, 모바일 환경에서 어떻게 동작하는지 예측이 안돼서 해당 부분 우선 머지된 이후 dev를 통해서 확인해보고 작업해야할 것 같습니다. 그래서 다른 기능(좋아요 탭 기능, 검색창 국가별 탭 기능) 작업하다 롤백해서 커밋에 해당 부분이 들어갔다가 빠졌습니다. 참고 바랍니다!
drawer 괜히 신경쓰여서 수정하였는데 엄연히 이 부분은 refactor에 갔어야 했을 것 같습니다 ㅠ 눈에 보여서 작업하다보니 해당 부분이 같이 들어갔네요. 이후 작업부터는 좀 더 꼼꼼하게 분리하도록 하겠습니다 :)

@simorimi simorimi added the FE label Oct 16, 2024
@simorimi simorimi added this to the sprint 6 milestone Oct 16, 2024
@simorimi simorimi self-assigned this Oct 16, 2024
@simorimi simorimi changed the title [Feature] - 프로필 이미지 수정, 여행기 등록 시 여행 장소마다 국가 코드 주는 기능 구현 [Feature] - 프로필 이미지 수정 기능 및 여행기 등록 시 여행 장소마다 국가 코드 주도록 구현 Oct 16, 2024
Copy link

github-actions bot commented Oct 16, 2024

Test Results

11 tests   11 ✅  14s ⏱️
 2 suites   0 💤
 1 files     0 ❌

Results for commit 0ec73d3.

♻️ This comment has been updated with latest results.

Copy link

Copy link

Copy link
Contributor

@jinyoung234 jinyoung234 left a comment

Choose a reason for hiding this comment

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

안녕하세요 시모~! 전체적인 코드는 확인했습니다!

현재 아래와 같은 문제를 확인했습니다.

  • 여행기 이미지를 삭제 한 상태에서 request를 날리면 500 에러를 반환함.
    • Default image로 요청을 보내야하는데 빈 문자열로 요청을 보내는 상황

�빈 문자열로 보낼 때 default image 처리만 해주면 될거 같아요!! 작업 고생 많으셨습니다 :)

@@ -14,7 +14,9 @@ import * as S from "./GoogleSearchPopup.styled";

interface GoogleSearchPopupProps {
onClosePopup: () => void;
onSearchPlaceInfo: (placeInfo: Pick<TravelTransformPlace, "placeName" | "position">) => void;
onSearchPlaceInfo: (
placeInfo: Pick<TravelTransformPlace, "placeName" | "position" | "countryCode">,
Copy link
Contributor

Choose a reason for hiding this comment

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

Pick<TravelTransformPlace, "placeName" | "position" | "countryCode"> 해당 타입에 대해 중복된 부분이 꽤나 발견되는거 같은데 대한 타입을 따로 분리하면 어떨까 싶긴 하네요~!
(countryCode 뿐만 아니라 다른 타입이 추가된다면 또 다 추가해야하니까 번거로워질 수 있을거 같아요)

Copy link
Author

Choose a reason for hiding this comment

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

좋은 피드백입니다! 해당 부분 PlaceInfo라는 타입으로 만들어 사용해줬습니다 고마워요

@@ -85,13 +85,11 @@ export const MainPageTraveloguesList = styled.ul`
gap: ${({ theme }) => theme.spacing.m};
`;

export const OptionContainer = styled.div`
export const OptionContainer = styled.button`
Copy link
Contributor

Choose a reason for hiding this comment

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

[단순 궁금증]

button 태그로 변경한 이유가 그냥 궁금하긴 합니다~! (접근성 때문인건가 싶긴 했어용)

Copy link

Choose a reason for hiding this comment

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

드래그가 가능하다는 것을 표현하기 위해 cursor css가 들어간거라서
이 경우 수정 전 코드가 더 시맨틱적으로 맞는거 같습니다!
(메인 페이지 웹 접근성 개선하면서 대공사를 하게되서 따로 다시 수정 커밋은 안해주셔도 될거같아용)

Copy link
Author

Choose a reason for hiding this comment

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

click 메서드이기에 이 부분은 button 이 맞다고 생각했어요! 이름이 Container라 전체 option들에 대한 컨테이너라고 착각할 수 있을 거 같은데 이 부분은 한 옵션을 감싸는 wrapper라고 받아들이는 것이 좋습니다! 그래서 클릭하는 옵션이기에 당연히 button이 맞다고 생각하였습니다

gap: ${(props) => props.theme.spacing.xl};
Copy link
Contributor

Choose a reason for hiding this comment

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

고생한게 느껴지는 CSS.. 👍👍👍

alert(error.message);
setNickname(data?.nickname ?? "");
};
const TAB_CONTENT = [
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 상수를 constants 파일로 빼는건 어떨까 제안드립니다!

Copy link

Choose a reason for hiding this comment

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

상수 분리한거 좋네욥~~

Copy link
Author

Choose a reason for hiding this comment

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

constants 파일로 분리하였습니다 :)

@@ -55,7 +55,7 @@ const MyTravelogues = ({ userData }: MyTraveloguesProps) => {
contentDetail={myTravelogues}
renderItem={({ id, thumbnailUrl, title, createdAt }) => (
<S.Layout onClick={() => handleClickTravelogue(id)}>
<AvatarCircle $size="medium" profileImageUrl={thumbnailUrl} />
Copy link
Contributor

Choose a reason for hiding this comment

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

잘 캐치해주셨네요!

const [isModifying, setIsModifying] = useState(false);
const [isProfileImageLoading, setIsProfileImageLoading] = useState(false);

const [isModalOpen, handleOpenModal, handleCloseModal] = useToggle();
Copy link
Contributor

Choose a reason for hiding this comment

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

현재 해당 hook에 상태가 많은 만큼 정말 필요한 상태가 아니라면 분리 하는 것도 방법이라고 생각해요~!

모달 상태는 바깥으로 빼도 충분할거 같은데(내부에서 쓰이지 않기 때문) 시모의 생각은 어떠신가요?!

Copy link
Author

@simorimi simorimi Oct 20, 2024

Choose a reason for hiding this comment

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

close 로직의 경우 내부에서 사용하고 있어서 이 부분은 그대로 남겨둘게요!


const [isModalOpen, handleOpenModal, handleCloseModal] = useToggle();

const handleClickEditModalOpenButton = () => handleOpenModal();
Copy link
Contributor

Choose a reason for hiding this comment

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

개인적으로 handleClickEditModalOpenButtonhandleClickEditModalCloseButton는 handleOpenModal이나 handleCloseModal 대신 넣어줘도 충분한거 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

굳이 늘릴 필요 없는 코드였는데 좋은 피드백 고마워요! 처음에 toggle을 안 썼다가 고치면서 해당 코드가 나온 거 같네요 :)

userProfile: { data, status, error },
profileImageFileInputRef,
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

시간이 충분하다면 이 훅을 조금 더 리팩터링 해보는 것도 좋을거 같단 생각이 들었어요,,, (너무 많은 책임들이 쏠려 있어서 그런지 코드 해석에 시간이 걸렸어요)

제 생각에는

  1. 프로필을 초기화 하는 책임(useEffect)
  2. 프로필 수정 모달에 대한 책임(handleClickEditModalCloseButton, handleClickProfileImageEditButton, handleClickProfileImageDeleteButton, handleClickEditModalOpenButton)
  3. 프로필 수정에 대한 책임(handleClickProfileEditButton, handleChangeProfileImage, handleLoadProfileImage, handleClickProfileEditConfirmButton, handleClickProfileEditCancelButton, handleChangeNickname)

정도로만 나눠도 괜찮을거 같다는 생각이 들었어요~!

Copy link
Author

@simorimi simorimi Oct 20, 2024

Choose a reason for hiding this comment

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

우선 states, handler 이런식으로 반환하던 것을 editModal, profileImage, profileNickname, ProfileEdit, userProfile 로 반환하고 해당 순서로 정리해 뒀습니다. 또한 해당 부분을 각각 훅으로 분리해서 훨씬 가독성이 좋아졌네요 좋은 피드백 감사합니다 :)

@@ -7,6 +7,11 @@ import { extractLastPath } from "@utils/extractId";
import * as S from "./SearchPage.styled";
import TravelogueList from "./TravelogueList/TravelogueList";

const TAB_CONTENT = [
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 constants 파일로 분리해보면 좋겠다는 생각이 들었어요!

Copy link
Author

Choose a reason for hiding this comment

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

고맙습니다 잘 분리했습니다

Copy link

@0jenn0 0jenn0 left a comment

Choose a reason for hiding this comment

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

시모 코드 잘 봤습니다!
고생하셨습니다! 👍

alert(error.message);
setNickname(data?.nickname ?? "");
};
const TAB_CONTENT = [
Copy link

Choose a reason for hiding this comment

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

상수 분리한거 좋네욥~~

@@ -85,13 +85,11 @@ export const MainPageTraveloguesList = styled.ul`
gap: ${({ theme }) => theme.spacing.m};
`;

export const OptionContainer = styled.div`
export const OptionContainer = styled.button`
Copy link

Choose a reason for hiding this comment

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

드래그가 가능하다는 것을 표현하기 위해 cursor css가 들어간거라서
이 경우 수정 전 코드가 더 시맨틱적으로 맞는거 같습니다!
(메인 페이지 웹 접근성 개선하면서 대공사를 하게되서 따로 다시 수정 커밋은 안해주셔도 될거같아용)

Copy link

@0jenn0 0jenn0 Oct 18, 2024

Choose a reason for hiding this comment

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

usePostUpload에서 한번에 처리하도록 리팩토링하셨군요!
이미지가 사용되는 컴포넌트에 따라서 최대로 렌더링되는 크기가 달라서
이 부분에서 이미지 resize에 대한 max width, max height를 받게하면 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

오 내부에 이러한 사항이 있는 줄 몰랐군요! 해당 부분 수정했습니다 고마워요!

setIsModalOpen(false);

const files = Array.from(e.target.files as FileList);
const profileImage = await mutateAddImage(files);
Copy link

Choose a reason for hiding this comment

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

사용자 프로필은 여행기 썸네일, 여행기 장소와 달리 크기가 작지만,
usePostUploadImages에서 똑같이 max width가 900대로 resize될텐데요
usePostUploadImages에서 max width, max height를 props로 받게해서 사용자 이미지는 레티나 디스플레이 대응해서 아바타 프로필 컴포넌트 최대 widht의 2배로 값 넣어주면 더 좋은 최적화가 될 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분 위와 마찬가지로 수정했습니다 고마워요 :)

Copy link

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants