feat: current generation 업데이트 #157

geongyu09 · 2024-08-19T12:05:21Z

주요 변경사항

새로운 신입모집이 시작됨에 따라서 CURRENT_GENERATION 을 28로 업데이트 했습니다.
이에 따라서 불러오는 상수 데이터를 저장하는 28.ts 및 기타 파일들을 추가하였습니다. 이는 27기의 파일과 동일하며, tf팀의 요청사항이 있을시 변경하도록 하겠습니다.
추가적으로 navbar의 탭을 리팩토링 하였습니다
- 기존 a 태그를 사용하여 전체 페이지를 리렌더링 하는 로직에서, Link 태그로 변경하여 next의 최적화 이점을 얻을 수 있도록 하였습니다.
- 현재 url을 가져오는 로직이 최상위 로직에서 props로 필요한 곳까지 전달하는 로직을 해당 컴포넌트에서 usePathname() 을 사용하여 가져올 수 있도록 하였습니다.
기존 칸반, 면접 기록, 지원 현황 페이지에서 각각 이동 방식이 [generation] 부분을 CURRENT_GENERATION에서 가져와 사용하던 부분을 현재 url에서 가져와 generation을 사용하는 방식으로 변경하여, 만약 이전 신입모집 기록을 볼 때 라우팅이 해당 신입모집 페이지만 볼 수 있도록 수정하였습니다.
이전 신입모집을 바로 볼 수 있도록 네비게이션바에 이전 신입모집 보기 버튼을 추가하였습니다. 이는 url에서 가져온 generation값이 CURRENT_GENERATION과 동일한지를 확인 후 이전 신입모집 보기 혹은 현재 신입모집 보기 버튼으로 분기처리하도록 하였습니다.

네브바 최적화

기존에 매번 전체 회면이 새롭게 렌더링 되던 부분을 변경된 부분만 렌더링 되도록 수정하였습니다

이전 신입모집 보기

리뷰어에게...

관련 이슈

closes #156

28기 application 및 applicant 상수 데이터 추가 #156

- 불필요하게 상위 컴포넌트에서 currentPath를 받아오던 로직 제거 - currentPath를 next의 usePathName() 으로 가져와서 사용할 수 있도록 수정 #156

isShort props 또한 불필요하게 props로 전달되던 것을 필요한 컴포넌트에서 훅으로 받아오도록 수정

이전 신입모집 기록을 볼 수 있다 라는 요구사항에 맞추어 이전 신입모집 보기 버튼을 추가 기존에 단순히 navbar를 그리기 위한 상수로서 사용하던 부분을 함수형태로 바꾸어 generation을 받아 href를 유동적으로 변경가능하도록 수정 단 #156

loopy-lim

너무 보기 힘들죠 ㅠㅠㅠ 그래도 조금씩 바꾸어 주셔서 감사합니다 ㅠㅠㅠㅠ

loopy-lim · 2024-08-21T01:25:14Z

frontend/app/(WithNavbar)/layout.tsx

-  const headersList = headers();
-  const header_url = headersList.get("x-url") || "";
-  const [_, __, currentPath, generation, ___] = header_url.split(/[/?]+/);
-  const isShort = currentPath === "kanban";
-
  return (
    <div className="px-24 min-w-[1280px] flex p-12">
-      <CommonNavbar
-        generation={generation}
-        currentPath={currentPath}
-        isShort={isShort}
-      />


여기서 데이터를 받아와 뿌려주어 생기는 리렌더링이었군요!!
다른 곳에서 navbar를 사용할 줄 알아, 위에서 내리도록 작성했는데 이는 성급한 일반화로 발전한 것 같네요

이제 안깜빡 거리나요 ㅎ 원래 네브바 나올때마다 깜빡였던거같던데

loopy-lim · 2024-08-21T01:54:01Z

frontend/components/common/navbar/NavbarCell.tsx

+  const currentPath = usePathname();
+  const currentType = currentPath.split("/")[1];
+  const isShort = currentType === "kanban";
+
+  const generation = currentPath.split("/")[2];


소소한... 레펙토링...

Suggested change

const currentPath = usePathname();

const currentType = currentPath.split("/")[1];

const isShort = currentType === "kanban";

const generation = currentPath.split("/")[2];

const [_, currentType, generation] = usePathname().split("/");

const isShort = currentType === "kanban";

바로 적용해보도록 하겠습니다!

geongyu09 · 2024-08-21T03:05:23Z

헉.. 리뷰까지.. 바쁘신 와중에도 pr 읽어주셔서 감사합니다!

- currentType을 가져오는 로직 리팩토링

smb0123 · 2024-08-21T03:59:32Z

frontend/app/kanban/[generation]/detail/page.tsx

-        columnIndex={+columnIndex ?? 0}
+        columnIndex={+(columnIndex ?? 0)}


바꾸기 전 코드와 바꾼 코드 둘 다 문자형을 숫자형으로 바꿔준다고 생각하는데 어떤 차이가 있나요?

KanbanColumnDetailCard 컴포넌트에서 columnIndex가 0으로 넘겨받는다면 useQuery에 enabled 속성을 넣어서 api 호출을 안하는게 좋아보입니다.

page.tsx에서 columnIndex를 props로 넘기는 것 대신에 KanbanColumnDetailCard 컴포넌트에서 아래와 같이 사용하면 props를 지울 수 있을 것 같아요

const searchParams = useSearchParams(); searchParams.get("columnIndex");

아이고 맞네요.. 수정하도록 하겠습니다!

제가 이전에 몇번의 삽질을 했었는데요,
그때 CURRENT_GENERATION 상수값을 .env로 관리하는 게 어떨지 생각하면서 변경했었습니다.
이렇게 되면 CURRENT_GENERATION의 값이 undefined가 될 수 있어서, 단순히 +columnIndex 를 하면 NaN 이 나올 수 있기에 위와같이 수정했었습니다.
결론적으로 현재는 .env로 관리하지 않기 때문에 해당 코드를 불필요하게 좋지 않은 방향으로 변경하게 되었네요..

2yunseong

고생하셨습니다 . 리뷰 한번만 확인해주세용 감사합니다

저로써 넥스트는 알다가도 모르는게 많네요ㅋ_ㅋ 코드 품질 위주로 리뷰 남겨보았습니다!

2yunseong · 2024-08-21T04:39:17Z

frontend/app/(WithNavbar)/layout.tsx

@@ -1,22 +1,12 @@
 import CommonNavbar from "@/components/common/navbar/Navbar";
 import { PropsWithChildren } from "react";


type import를 사용해봅시다 :)

Suggested change

import { PropsWithChildren } from "react";

import { type PropsWithChildren } from "react";

넵 수정하겠습니다!

2yunseong · 2024-08-21T04:56:39Z

frontend/components/common/navbar/Navbar.tsx

-          generation={generation}
-          isShort={isShort}
-        />
+        <NavbarToggle />


Toggle이라.. 맞는거 같지만 더 좋은 이름이 없을까유?? 항상 이런 부분이 어려운거 같네요 ㅠㅠ

맞아요.. 사실 토글이긴 한데 보여질 기수를 변경해주는 토글이니 NavbarGenerationToggle 은 어떠신가요..?

2yunseong · 2024-08-21T05:04:47Z

frontend/components/common/navbar/NavbarCell.tsx

  item: {
-    type: string;
+    type: string; //TODO: 타입 정의하기
    href: string;
    target: string;
    short_title: string;
    title: string;
  };


~~item으로 한번 감싸진 이유가 무엇인가요?~~ 이전의 코드로 인한거군요 !
추가로 short_title에서 파이썬의 향기가 나네요ㅋ_ㅋ

2yunseong · 2024-08-21T05:06:25Z

frontend/components/common/navbar/NavbarCell.tsx

+const CommonNavbarCell = ({ item }: CommonNavbarCellProps) => {
+  const [_, currentType] = usePathname().split("/");
+  const isShort = currentType === "kanban";
+
  const linkButtonClassName =
    "flex justify-between p-4 hover:bg-secondary-100 hover:text-white rounded-lg";
+
+  const { href, short_title, target, title, type } = item;


item의 depth를 줄이는 걸 추천을 드리나 혹여나 구조 변경이 어려우시다면 destructure는 어떠신지 여쭤봅니당

Suggested change

const CommonNavbarCell = ({ item }: CommonNavbarCellProps) => {

const [_, currentType] = usePathname().split("/");

const isShort = currentType === "kanban";

const linkButtonClassName =

"flex justify-between p-4 hover:bg-secondary-100 hover:text-white rounded-lg";

const { href, short_title, target, title, type } = item;

const CommonNavbarCell = ({ item:{ href, short_title, target, title, type }}: CommonNavbarCellProps) => {

const [_, currentType] = usePathname().split("/");

const isShort = currentType === "kanban";

const linkButtonClassName =

"flex justify-between p-4 hover:bg-secondary-100 hover:text-white rounded-lg";

넵 위 커맨트와 함께 수정해보도록 하겠습니다!

2yunseong · 2024-08-21T05:08:11Z

frontend/components/common/navbar/NavbarCell.tsx

+  const [_, currentType] = usePathname().split("/");
+  const isShort = currentType === "kanban";


추가로 CommonNavbarCell을 사용하는 부모 컴포넌트가 모두 usePathname을 호출하는데, 자식 컴포넌트인 이곳에서 호출하기 보다는 props로 받아오는 건 어떤가요 ?

넵! 해당 부분이 불필요하게 반복되는 문제가 있네요.. 수정하겠습니다!

2yunseong · 2024-08-21T05:13:14Z

frontend/components/common/navbar/NavbarCell.tsx

+      className={cn(
+        currentType === type


이렇게는 안될까용?

className={cn( linkButtonClassName, currentType === type && "!bg-black !text-white" )}

수정해보겠습니다!

2yunseong · 2024-08-21T05:14:14Z

frontend/components/common/navbar/NavbarCell.tsx

-      key={item.type}
+      )}
+      href={href}
+      target={target === "_blank" ? "_blank" : ""}


여기서 조건부로 연산하는 것 (target) 보다 props로 받아오는 타입을 강제하는 건 어떤가요?

좋습니다! props로 받을 수 있는 값의 타입을 정의해보도록 하겠습니다!

상위 컴포넌트인 page.tsx에서 받아오던 columnIndex를 제거하였습니다.

#156

…Toggle.tsx) #156

loopy-lim · 2024-08-21T05:41:45Z

frontend/public/icons/lt.icon.white.svg

엇... 놓친거긴 한데... icon의 이미지 크기는 일정할 수록 좋아요!!

아.. 해당 부분을 수정한 이유는 흰색 > 과 회색 > 아이콘이 크기가 다른 문제가 있었기 때문입니다.
(정확히는 회색은 패딩(?) 이 없었지만, 흰색은 존재하여 문제가 있다고 판단하였습니다..)

뭔가 둘을 동일하게 두는게 좋지 않을까 해서 수정을 하게 되었습니다..!

혹시 해당 부분을 어떤 방향성으로 수정하는 것이 좋을까요..?

그렇다면 개인적으로는 여기에 나와 있는 기준대로 16px 또는 24px로 통일합시다... ㅠㅠㅠ
안되어 있는 부분은 제가 잘못한 것이므로 잘 처리 해주시면 감사하겠습니다 ㅠㅠ

답변이 늦어서 죄송합니다.. 해당 부분은 따로 pr을 날리거나 이슈를 파도록 하겠습니다!

불필요한 로직 제거 #156

불필요하게 한 뎁스 더 감쌓있던 부분 리팩토링 #156

geongyu09 · 2024-08-21T07:52:10Z

피드백 받은 부분을 전체적으로 수정해보았습니다!

cn 사용 로직 개선
props 불필요한 depth 줄이기
중복된 코드 props로 받아오기

2yunseong

good! 고생하셨습니다 :)

smb0123

고생하셨습니다 !

BlackBean99

지나가는 엔지니어입니다. 지나가겠습니다 호닥닥

BlackBean99 · 2024-08-23T13:38:59Z

frontend/components/common/navbar/NavbarGenerationToggle.tsx

+export const NavbarGenerationToggle = ({
+  generation,
+  isShort,
+}: NavbarGenerationToggleProps) => {
+  const isCurrentGeneration = +generation === CURRENT_GENERATION;
+  const targetGeneration = isCurrentGeneration
+    ? CURRENT_GENERATION - 1
+    : CURRENT_GENERATION;


1씩 줄여가며 찾는것도 있지만 option 에서 몇기인지 셀랙 하는건 어떨까요? 고냥 opnion..
저번기수 기준도 중요하지만 저저번 기수의 피드백이 가끔은 선배들의 혜안?을 엿볼때 좋더라구요..

현업자 리뷰.. 너무 귀하네요..!
동의힙니다! 다만, 개인정보를 1년동안만 가지고 있는다는 정책이 있어서 지난 모집까지만 볼 수 있도록 요구사항이 만들어 진 것으로 알고 있습니다!

BlackBean99 · 2024-08-23T13:40:21Z

frontend/app/(WithNavbar)/layout.tsx

-  const headersList = headers();
-  const header_url = headersList.get("x-url") || "";
-  const [_, __, currentPath, generation, ___] = header_url.split(/[/?]+/);
-  const isShort = currentPath === "kanban";
-
  return (
    <div className="px-24 min-w-[1280px] flex p-12">
-      <CommonNavbar
-        generation={generation}
-        currentPath={currentPath}
-        isShort={isShort}
-      />


이제 안깜빡 거리나요 ㅎ 원래 네브바 나올때마다 깜빡였던거같던데

BlackBean99 · 2024-08-23T13:40:56Z

frontend/app/kanban/[generation]/detail/page.tsx

@@ -47,12 +47,11 @@ const DetailContentJunction = ({

 const KanbanBoardDetailPage = ({
  params: { generation },
-  searchParams: { columnIndex, applicantId, type, cardId },
+  searchParams: { applicantId, type, cardId },


type이 뭘 지칭하는 타입인지 domain을 명시하면 좋을 것 같아요! cardId는 card의 id잖아요.

음..! 좋네요! 해당 부분의 명확한 이름을 고민해보겠습니다!

BlackBean99 · 2024-08-23T13:42:48Z

frontend/src/constants/index.ts

@@ -1,4 +1,4 @@
-export const CURRENT_GENERATION = 27;
+export const CURRENT_GENERATION = 28;


백엔드쪽도 이게 고민이긴 한데 신입 기수를 변수로 저장하는게 매번 기수마다 코드를 건드려야 한다는 심각한 문제가 있습니다.
서버에서 자체적으로 관리를 하거나 공식을 만들어드리려고 하는데 (혹여나 1년에 1기수만 뽑게되면 어쩌나 하는 걱정때문에요..)

오 좋은것 같습니다!
매번 새로운 기수를 뽑게될 때마다 코드를 변경해야하는 비용을 줄이는 방법을 함께 고민해볼 필요성이 있을 것 같습니다!

geongyu09 added 3 commits August 19, 2024 20:24

chore: current_generation 최신화

aa8e604

chore: current_generation 최신화에 따른 필요한 파일들 추가

92039da

28기 application 및 applicant 상수 데이터 추가 #156

refactor: commonNavbar 리팩토링(NavbarCell.tsx)

e61bbc3

- 불필요하게 상위 컴포넌트에서 currentPath를 받아오던 로직 제거 - currentPath를 next의 usePathName() 으로 가져와서 사용할 수 있도록 수정 #156

geongyu09 requested review from BlackBean99 and loopy-lim as code owners August 19, 2024 12:05

geongyu09 added 3 commits August 20, 2024 00:20

refactor: isShort 또한 상위에서 받아오는 것이 아닌 필요한 곳에서 사용하도록 수정

0301d45

isShort props 또한 불필요하게 props로 전달되던 것을 필요한 컴포넌트에서 훅으로 받아오도록 수정

chore: useCreateQueryString 훅 캐리지리턴 제거

cf534d4

geongyu09 self-assigned this Aug 20, 2024

fix: 빌드 버그 수정

52f4ff1

loopy-lim reviewed Aug 21, 2024

View reviewed changes

geongyu09 requested review from smb0123 and 2yunseong August 21, 2024 03:05

refactor: NavbarCell 리팩토링(NavbarCell.tsx)

72d58e6

- currentType을 가져오는 로직 리팩토링

geongyu09 changed the title ~~chore: current generation 업데이트~~ feat: current generation 업데이트 Aug 21, 2024

smb0123 reviewed Aug 21, 2024

View reviewed changes

2yunseong requested changes Aug 21, 2024

View reviewed changes

geongyu09 added 3 commits August 21, 2024 14:19

refactor: kanban 로직 리팩토링(ColumnWithBackButton.tsx)

e9c5ec5

상위 컴포넌트인 page.tsx에서 받아오던 columnIndex를 제거하였습니다.

refactor: type import 적용(layout.tsx)

a702b1e

#156

refactor: NavbarToggle 이름 NavbarGenerationToggle로 변경(NavbarGeneration…

df9f627

…Toggle.tsx) #156

loopy-lim reviewed Aug 21, 2024

View reviewed changes

geongyu09 added 4 commits August 21, 2024 14:51

refactor: NavbarCell classname 리팩토링(NavbarCell.tsx)

b2a22ef

불필요한 로직 제거 #156

refactor: 불필요한 props depth 줄이기 (NavbarCell.tsx)

f9343d9

불필요하게 한 뎁스 더 감쌓있던 부분 리팩토링 #156

refactor: 네비게이션바 상수데이터 타입 정의

3170bf6

refactor: 중복된 코드 props으로 받아도로고 수정(Navbar.tsx)

fa560d4

geongyu09 requested a review from 2yunseong August 21, 2024 07:50

geongyu09 added the feature 기능개발 label Aug 21, 2024

2yunseong approved these changes Aug 23, 2024

View reviewed changes

smb0123 reviewed Aug 23, 2024

View reviewed changes

smb0123 approved these changes Aug 23, 2024

View reviewed changes

BlackBean99 approved these changes Aug 23, 2024

View reviewed changes

geongyu09 merged commit c597b6e into main Aug 24, 2024
1 check passed

geongyu09 deleted the feat/156-generation-update branch August 24, 2024 11:34

		columnIndex={+columnIndex ?? 0}
		columnIndex={+(columnIndex ?? 0)}

		@@ -1,22 +1,12 @@
		import CommonNavbar from "@/components/common/navbar/Navbar";
		import { PropsWithChildren } from "react";

	import { PropsWithChildren } from "react";
	import { type PropsWithChildren } from "react";

		const [_, currentType] = usePathname().split("/");
		const isShort = currentType === "kanban";

		@@ -1,4 +1,4 @@
		export const CURRENT_GENERATION = 27;
		export const CURRENT_GENERATION = 28;

feat: current generation 업데이트 #157

feat: current generation 업데이트 #157

Conversation

geongyu09 commented Aug 19, 2024 • edited Loading

주요 변경사항

네브바 최적화

이전 신입모집 보기

리뷰어에게...

관련 이슈

loopy-lim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geongyu09 commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2yunseong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2yunseong Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geongyu09 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geongyu09 commented Aug 21, 2024

2yunseong left a comment

Choose a reason for hiding this comment

smb0123 left a comment

Choose a reason for hiding this comment

BlackBean99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geongyu09 commented Aug 19, 2024 •

edited

Loading

2yunseong Aug 21, 2024 •

edited

Loading

geongyu09 Aug 21, 2024 •

edited

Loading