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

[분실물] 공지 분실물 기능 #640

Merged
merged 46 commits into from
Jan 25, 2025
Merged

Conversation

junghaesung79
Copy link
Contributor

What is this PR? 🔍

Changes 📝

  • 총학생회 계정으로 분실물 글쓰기 진입점

  • 공지사항 분실물 글쓰기

  • 분실물 관련 로깅

  • 공지 관련 경로 수정

  • 로깅 프로퍼티 키워드 변경

  • 분실물 자세히 보기

ScreenShot 📷

Test CheckList ✅

  • test 1
  • test 2
  • test 3

Precaution

✔️ Please check if the PR fulfills these requirements

  • It's submitted to the correct branch, not the develop branch unconditionally?
  • If on a hotfix branch, ensure it targets main?
  • There are no warning message when you run yarn lint

@junghaesung79 junghaesung79 self-assigned this Jan 23, 2025
Copy link
Contributor

@hyejun0228 hyejun0228 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~!
급한 일정인 것 같아 일단 눈에 보이는 이슈만 빠르게 적어놓겠습니다. 위사진이 피그마, 밑이 로컬입니다

  1. 높이 맞추기
image image
  1. 분실물 수 추가하기
image
  1. 습득물 물품 등록 간의 간격이 너무 큰것같아요! gap 과 padding-top이 같이 잡혀있어요 하나 삭제해주세요!
image image

@hyejun0228
Copy link
Contributor

추가로 내용 입력시 최대 입력 설정도 부탁드려요!

Copy link
Contributor

@JeongWon-CHO JeongWon-CHO left a comment

Choose a reason for hiding this comment

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

수고하셨습니다 !!
주어진 시간이 짧았음에도 다 완성하시는 모습이 멋진 거 같아요👍👍😄

Comment on lines +4 to +7
// import FoundIcon from 'assets/svg/Articles/found.svg';
// import LostIcon from 'assets/svg/Articles/lost.svg';
// import CloseIcon from 'assets/svg/Articles/close.svg';
// import useBooleanState from 'utils/hooks/state/useBooleanState';
Copy link
Contributor

Choose a reason for hiding this comment

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

사용하지 않는 아이콘과 훅을 남겨두신 이유가 있을까요 ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

다음 스프린트에 들어갈 svg들을 미리 import 해두었습니다!


const handleRightButtonClick = () => {
setImage(images[(imageIndex + 1) % images.length]);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

handleLeftButtonClickhandleRightButtonClick 함수의 로직이 거의 비슷한데, 이를 하나의 함수로 만드는 것은 어떻게 생각하시나요 ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

확실히 1 또는 -1을 인자로 받는 함수로 바꿀 수 있을 것 같아요!

<div className={styles.contents__navigation}>
{images.length > 1 && Array.from({ length: images.length }).map((_, index) => (
<button
key={uuidv4()}
Copy link
Contributor

Choose a reason for hiding this comment

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

이미지 배열의 id 값을 키로 사용하지 않은 이유는 무엇인가요 ?? 😲

Copy link
Contributor Author

@junghaesung79 junghaesung79 Jan 25, 2025

Choose a reason for hiding this comment

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

처음에는 같은 이미지를 올리면 url이 같아져서 중복 키가 될 것이라고 생각했어요. 그런데 다시 생각해보니 unsigned url은 다른 주소를 반환하기 때문에 중복이 일어나지 않는군요! 감사합니다~

Comment on lines 93 to 94
width: 127px;
height: 48px;
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
Contributor Author

Choose a reason for hiding this comment

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

질문의 의도를 제대로 이해하지 못했어요😥 다시 한 번 설명해주실 수 있을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

paddingmargin으로 조절하지 않고, 고정된 크기인 widthheight로 크기를 설정한 이유에 대한 질문이었습니다 ..!
반응형 UI에서는 paddingmargin을 이용하여 요소가 유동적으로 변경되도록 하는 게 좋다고 알고 있었기 때문에 해당 질문을 드렸어요 !! 🫠🫠

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 품목 추가 버튼은 뷰포트 너비에 영향을 받을 필요성을 느끼지 못했기 때문에 고정된 px로 만들었어요! 그리고 글자가 가운데에 오도록 하는 것은 justify-content, align-items로 맞췄구요!

@@ -0,0 +1,71 @@
import useDeleteLostItemArticle from 'pages/Articles/hooks/useDeleteLostItemArticles';
Copy link
Contributor

Choose a reason for hiding this comment

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

파일명 오타인 거 같습니다...! indes.tsx 이 맞나요 ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오타가 있었군요! 감사합니다!

Comment on lines 29 to 34
const handleConfirmDeleteClick = () => {
logFindUserDeleteConfirmClick();
deleteArticle(articleId);
navigate(ROUTES.Articles(), { replace: true });
closeDeleteModal();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

삭제에 성공했을 때(deleteArticle을 성공적으로 불러왔을 때)만 navigate를 하면 어떨까요 ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onSuccess 시에만 해당 동작이 일어나도록 수정하였습니다!
9497078

@junghaesung79 junghaesung79 merged commit 3bd5ddf into develop Jan 25, 2025
2 checks passed
@junghaesung79 junghaesung79 deleted the feat/#630/lostitem branch January 25, 2025 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[분실물] 공지 분실물 기능
3 participants