Skip to content

Conversation

@Nahyun-Kang
Copy link
Contributor

@Nahyun-Kang Nahyun-Kang commented Dec 15, 2024

개요

  • 개요는 변경 사항 및 관련 이슈에 대해 간단하게 작성해주세요.
  • 해당 PR에 대한 리뷰어와 라벨을 생성해 주세요.

작업 사항

@ParkSohyunee

  • 요청주제관리 페이지를 기존의 admin-temp-topics 폴더에서 소현님께서 새로 생성하신 admin > topics 폴더에 이동시켰습니다.
  • 소현님께서 구현하신 어드민의 게시물관리 페이지 ui를 참고하여 수정했습니다.
  • 조회 API 연동, 수정 로직을 추가했습니다.

참고 사항 (optional)

  • 작업 중 만난 버그, 구현 과정 중 어려움, 고민
  • 해결 방법
  • 해당 라이브러리를 선택한 이유
  • 테스트 계획

관련 이슈 (optional)


스크린샷

ezgif-3-b7ff008e95


리뷰어에게

  • 어떤 부분에 리뷰어가 집중하면 좋을지

@vercel
Copy link

vercel bot commented Dec 15, 2024

@Nahyun-Kang is attempting to deploy a commit to the Eujin Ahn's projects Team on Vercel.

A member of the Team first needs to authorize it.

@Nahyun-Kang Nahyun-Kang reopened this Dec 15, 2024
Copy link
Contributor

@ParkSohyunee ParkSohyunee left a comment

Choose a reason for hiding this comment

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

나현님, 어드민 요청주제 페이지 구현하시느라 고생많으셨습니다! 🥹✨ 리뷰 남긴 부분 확인 부탁드립니다~! 감사합니다. 💕LGTM💕

Comment on lines 6 to 9
export const body = style({
width: '100vw',
width: '100%',
minHeight: '100vh',
padding: '16px 16px 120px',

Copy link
Contributor

Choose a reason for hiding this comment

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

아래처럼 높이가 보여져서 수정이 필요할 것 같아요.

    width: 100%;
    height: 100%; 
    padding: 1.5rem; // 패딩 수정
    /* position: relative; */ //제거
    /* overflow-y: auto; // 제거

image

Comment on lines +91 to +102
export const table = style({
maxWidth: '850px',
padding: '1rem',

display: 'flex',
flexDirection: 'column',
gap: '1rem',

backgroundColor: vars.color.white,
borderRadius: '8px',
});

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

Choose a reason for hiding this comment

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

@Nahyun-Kang 나현님, 혹시 해당 리뷰는 반영되었을까요? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ParkSohyunee 네 소현님! 필요없는 css 코드는 삭제해두었습니다!👀

Copy link
Contributor

Choose a reason for hiding this comment

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

해당 파일은 삭제해야 하는 파일이 맞을까요?! 👀

Comment on lines 56 to 58
<td className={styles.buttons}>
<span className={styles.rowText}>{topic?.isAnonymous ? 'O' : 'X'}</span>
</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 나현님, 클론 받아서 테스트해보니 요청 주제를 '익명'으로 만들었을때도 리스폰스에서는 isAnonymous 필드 값이 false로 보여집니다. 이 부분 수정이 필요할 것 같아요~!
  2. 요청 주제 생성시 노출값이 default로 true인 것이 맞을까요? 검토 후 보여지는 플로우인것 같아서 확인차 여쭤봅니다.
  3. (의견) 테이블 제목을 '익명' 대신 만든사람 닉네임을 두는 것이 어떨까요? 리스폰스로 해당값이 내려가기도 하고, 관리자가 주제 요청한 사용자에 대해 알아야할 것 같아서요~! 다만, 익명일때만 사용자아이디(익명 요청) 이런식으로 표현해도 될 것 같아요,,

Copy link
Contributor Author

@Nahyun-Kang Nahyun-Kang Jan 5, 2025

Choose a reason for hiding this comment

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

  1. 소현님 2번에 대하여, 어드민 페이지가 완전하게 만들어지지 않았을 때 홈 화면에 요청주제가 뜨게 하기 위해서 잠시 회의에서 isAnonymous 옵션을 모두 false로 하고 바로 보여지게 한 걸로 알고 있습니다! 이 부분은 해당 페이지가 완성되면 동호님께 요청드릴 예정입니다! @ParkSohyunee

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번에 대하여 테스트를 해보니, 익명 요청했는데 false로 보여지네요. 이 부분은 요청 주제 생성시와 관련되어있는 것 같아서, 요청 주제 생성 페이지 담당자의 검토도 필요할 것으로 보입니다..!

Comment on lines +64 to +69
<td>
<select onChange={handleToggleExposeButton} value={topic?.isExposed ? '공개' : '비공개'}>
<option>공개</option>
<option>비공개</option>
</select>
</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

(의견) 혹시 요청 주제 관리 페이지에서 토글로 노출상태를 바로 변경하는 기능을 두신 이유가 무엇인지 궁금합니당 👀
공지의 경우는 생성/수정 필드와 노출 관련 API가 분리되어 있는 상황이었는데, 요청 주제 관리는 수정 바텀시트에서 함께 수정할 수 있을 것 같다는 생각이 들어서요~! 중복 코드도 제거할 수 있고, 사용성 면에서도 불편함이 없을 것 같아서 제안드려봅니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

소현님 이 부분은 와이어프레임을 보고 그대로 만든 건데 바텀시트 안에 들어있어도 좋을 것 같습니다! 다만 이번 PR 말고 다른 PR로 따로 올려서 서영님과 상의를 거친 후 수정해보겠습니다!

Comment on lines 22 to 23
const [isBottomSheetOpen, setIsBottomSheetOpen] = useState(false);

Copy link
Contributor

Choose a reason for hiding this comment

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

useBooleanOutput() 훅을 사용해서 추후 리팩토링하면 좋을 것 같습니당✨👍

Comment on lines +21 to +23
//페이지네이션 코드
// const pages = useMemo(() => Array.from({ length: 5 }, (_, idx) => idx + 1), []);

Copy link
Contributor

Choose a reason for hiding this comment

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

📌

Comment on lines 67 to 70
<tbody>
{topics && topics?.topicsList.map((topic, index) => <AdminTopicBox key={index} topic={topic} />)}
</tbody>
</table>
Copy link
Contributor

Choose a reason for hiding this comment

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

나현님, key는 index 대신 id로 수정하는 것이 안전해보입니다. 또한, topic의 타입추론이 안되는데 getAdminTopics API 함수의 리스폰스 타입을 지정해두면 좋을 것 같습니당

Comment on lines +36 to +48
const convertCategoryKorNameToCode = (korName: string) => {
const category = categories?.find((cat) => cat.korName === korName);
return category ? category.code : null; // 찾지 못하면 null 반환
};

const editTopicMutation = useMutation({
// mutationFn: () =>
// editAdminTopic({
// isExposed,
// title,
// categoryCode,
// }),
mutationFn: () =>
editAdminTopic({
topicId,
isExposed,
title,
categoryCode: convertCategoryKorNameToCode(selectedCategory as string) || '',
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

추후 리팩토링 하실 때 드롭다운을 ul, li태그가 아닌 select 태그를 사용하는 것으로 수정하면 카테고리 선택하고, 뮤테이션에 넣어주는 부분에 대해 코드 가독성면에서 좋을 것 같아요, 아래 코드 참고 부탁드립니당

// 제거
// const convertCategoryKorNameToCode = (korName: string) => {
//  const category = categories?.find((cat) => cat.korName === korName);
//  return category ? category.code : null; // 찾지 못하면 null 반환
//  };

  const editTopicMutation = useMutation({
    mutationFn: () =>
      editAdminTopic({
        ...
        categoryCode: selectedCategory, // 수정
      }),
    ...
    },
  });

return (
   <select ... onChange={(e: ChangeEvent<HTMLSelectElement>) => setSelectedCategory(e.target.value)}>
    {categories?.map((category) => (
      <option ... value={category.code}>{category.korName}</option>))}
  </select>
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

리팩토링 시 꼭 코드 반영하겠습니다. 조언 감사합니다 소현님!

@Nahyun-Kang Nahyun-Kang merged commit 8fad158 into 8-Sprinters:dev Jan 5, 2025
1 of 2 checks passed
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