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

여행 아이템 정보 수정/삭제 기능 구현 #133

Merged
merged 12 commits into from
Jul 26, 2023
Merged

Conversation

ashleysyheo
Copy link
Collaborator

@ashleysyheo ashleysyheo commented Jul 25, 2023

📄 Summary

Screenshot 2023-07-25 at 1 12 22 PM
  • 여행 수정 시 전에 입력했던 정보가 디폴트로 입력되어 있다.
  • 여행 삭제 시 목록에서 아이템이 사라진다.

+

  • Day Log 소제목을 수정할 수 있다. (비동기 로직 추가)

🙋🏻 More

close #126

@ashleysyheo ashleysyheo added ✨ Feature FE-Service 행록 서비스 프론트엔드 labels Jul 25, 2023
@ashleysyheo ashleysyheo added this to the 3차 스프린트 milestone Jul 25, 2023
@ashleysyheo ashleysyheo self-assigned this Jul 25, 2023
<MenuItem onClick={() => {}}>수정</MenuItem>
<MenuItem onClick={() => {}}>삭제</MenuItem>
<MenuItem onClick={openModal}>수정</MenuItem>
<MenuItem onClick={handleTripItem}>삭제</MenuItem>
Copy link
Collaborator

Choose a reason for hiding this comment

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

handleTripItemDelete은 어떤가유.. 하는 일도 그거 한가지라..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

헐.... handleTripItemDelete라고 한 줄 알았어요...... 수정하겠습니다!

frontend/src/hooks/trip/useAddTripItemForm.ts Outdated Show resolved Hide resolved
frontend/src/mocks/handlers/tripItem.ts Show resolved Hide resolved
Copy link
Collaborator

@Dahyeeee Dahyeeee left a comment

Choose a reason for hiding this comment

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

수정삭제까지 뚝딱~~~ 👍🏻

Copy link
Member

@dladncks1217 dladncks1217 left a comment

Choose a reason for hiding this comment

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

고생 많으셨어요 애슐리! 아주 잘 동작하는군요👍👍💯💯💯

이유는 모르겠지만 스타일이 안먹는 부분이 하나 있고, UI적인 부분에서 제안할 게 있습니다!
당장 수정해달라 그런건 아니고, 일단 여기에 남겨만 두고자 합니다 편하게 확인해주세요 :)

이 외로 코멘트 조금 남겼습니다!

스크린샷 2023-07-25 오후 5 46 20

애슐리 화면에서는 패딩이 잘 먹지만 제 화면에선 이상하게 안먹는 것 같습니다🥲

이거랑 이미지 넣는 부분에서 스크롤 바를 제거하는건 UX적인 면에서 별로 좋지 않으려나요?!
스크린샷 2023-07-25 오후 5 48 11

@@ -20,7 +20,7 @@
"@tanstack/react-query": "^4.29.19",
"axios": "^1.4.0",
"dotenv": "^16.3.1",
"hang-log-design-system": "^1.1.25",
"hang-log-design-system": "^1.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

드디어 1.2.0 ...!!ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그런데 곧 1.2.1 갈듯...

Copy link
Member

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋㅋㅋ오케 좋습니다

@@ -0,0 +1,17 @@
import { END_POINTS } from '@/constants/api';
Copy link
Member

Choose a reason for hiding this comment

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

경로 확인해주세요!

@@ -1,3 +1,5 @@
import TripItemAddModal from '@/components/trip/TripItemAddModal/TripItemAddModal';
Copy link
Member

Choose a reason for hiding this comment

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

경로 확인해주세요!

@@ -1,3 +1,5 @@
import TripItemAddModal from '@/components/trip/TripItemAddModal/TripItemAddModal';
import { useTripDates } from '@/hooks/trip/useTripDates';
Copy link
Member

Choose a reason for hiding this comment

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

경로 확인해주세요!

export const useDayLogTitleMutation = () => {
const dayLogTitleMutation = useMutation(patchDayLogTitle(), {
onError: () => {
alert('오류가 발생했습니다. 잠시 후 다시 시도해주세요.');
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
Collaborator Author

Choose a reason for hiding this comment

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

좋아요!

@@ -48,6 +66,7 @@ const TripItemAddModal = ({ tripId, dayLogId, dates, onClose }: TripItemAddModal
<PlaceInput
Copy link
Member

Choose a reason for hiding this comment

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

수정해달라는 코멘트는 아닙니다!

이 부분 코드를 어찌저찌 읽긴 했지만 코드 읽기가 쉽지 않더라구요🥲
updateInputValueuseAddTripItemForm에서 시작해서 PlaceInput으로 간 뒤 PlaceInput에서 useTripItemPlaceInput훅을 사용하기 위해 인자로 넣어주고 있어요...
최종적으로 handlePlaceChange를 사용하기 위해 4번을 거쳐 가는지라 이해하기 쉽지 않더군요🥲 제네릭 타입이라 중간중간에 타입 파일로도 확인하러도 계속 이동하고요 흑흑
updateInputValue라는 네이밍도 뭔가 값일것같은데 함수인 것도 살짝 어색한 것 같구요...!

애슐리 워낙 타입스크립트 잘 다루면서 코드도 잘 짜주시다 보니 물론 제 코드 해석력이 문제일수도 있지만요🥲
이 부분도 개선할 수 있을까요?!
당장 기능은 아주 잘 동작하니 꼭 개선해달라 그런건 아닙니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저도 이 부분이 좀 마음에 안 들어요...... 🥲 그런데 어떻게 처리하느게 좋을지 모르겠어요. 뭔가 hook으로 분리를 안 하면 컴포넌트 내부에 로직이 너무 많아지고, 보이는 부분에만 집중하지 못하고, 또 커스텀 훅으로 분리하면 지금처럼 미친듯한 depth가 발생하고요... 지금은 어떻게 하면 좋을지 잘 모르겠어서 이건 리팩토링하면서 조금 더 고민해볼게요!

함수명 updateInputValue는 인풋 값을 업데이트한다는 의미로 그렇게 명명했어요! 어떤 부분에서 네이밍이 어색한가요?!

Copy link
Member

Choose a reason for hiding this comment

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

흠 지금보니까 괜찮은거같기도...?🤔ㅋㅋㅋㅋㅋㅋㅋ
이대로 갑시다 네이밍

@ashleysyheo
Copy link
Collaborator Author

이거랑 이미지 넣는 부분에서 스크롤 바를 제거하는건 UX적인 면에서 별로 좋지 않으려나요?!

스크린샷 2023-07-25 오후 5 48 11

@Dahyeeee 헤다는 이 부분에 대해 어떻게 생각하세요?? 저는 끝이 애매하게 잘려서 사용자가 스크롤이 있다는 것을 인지하지 못 할 수도 있다고 생각해서 overflow: scroll을 추가했어요! 그런데 overflow scroll 말고 overflow: auto로 했어야 했네요... 행록 1.2.1 갑니다.................................

@Dahyeeee
Copy link
Collaborator

스크롤바 없으면 마우스로 스크롤하기가 불가능하기도 하고 애슐리 말대로 사용자가 스크롤 가능한지 모를 수도?!

스크롤바 없으면 데스크탑 마우스로 스크롤하기 불가능한 건 사실 citySearchBar에서도 마찬가지인긴 한데 citySearchBar는 스크롤바생기면 좀 뚱뚱 못생겨져서 일단 그냥 두는걸로..?

Copy link
Member

@dladncks1217 dladncks1217 left a comment

Choose a reason for hiding this comment

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

💯💯💯

@@ -20,7 +20,7 @@
"@tanstack/react-query": "^4.29.19",
"axios": "^1.4.0",
"dotenv": "^16.3.1",
"hang-log-design-system": "^1.1.25",
"hang-log-design-system": "^1.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋㅋㅋ오케 좋습니다

@@ -48,6 +66,7 @@ const TripItemAddModal = ({ tripId, dayLogId, dates, onClose }: TripItemAddModal
<PlaceInput
Copy link
Member

Choose a reason for hiding this comment

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

흠 지금보니까 괜찮은거같기도...?🤔ㅋㅋㅋㅋㅋㅋㅋ
이대로 갑시다 네이밍

@ashleysyheo ashleysyheo merged commit dd37778 into develop Jul 26, 2023
1 check passed
@ashleysyheo ashleysyheo deleted the feature/#126 branch August 1, 2023 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE-Service 행록 서비스 프론트엔드 ✨ Feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

여행 아이템 정보 수정/삭제 기능 구현
3 participants