[SP0] Recruit 탭 추가 #157

SeojinSeojin · 2023-09-02T00:25:47Z

Summary

close 리크루팅 탭 추가 #153

Screenshot

sopt.org/recruit
에 이미 배포되엇씁니다 ..

Comment

이미 머지된 코드라 엄청 빨리 보지는 않아도 괜찮아요!!
처음 리뷰인 만큼 궁금한 점들 모두 질문 주셔요~!

solar3070

첫 코드 리뷰입니다! 🥳

처음으로 공홈 코드를 봤는데 아무래도 여러 개발자가 오랫동안 개발하다 보니 코드의 통일감이 많이 떨어지는 것 같습니다. 앞으로 다른 기수들도 들어오고 더 많은 사람이 공홈을 개발하게 될 것을 고려한다면 3기에서 한 번 컨벤션을 잡고 가는 게 좋을 것 같습니다!

리크루팅 페이지 개발 고생하셨습니다~! 서진 짱

추가로 ActivityReview 관련 코드는 MainPage와 RecruitPage에서 중복되는 게 많아서 하나로 통일해도 좋을 것 같아요~

solar3070 · 2023-09-02T08:26:52Z

src/components/Header/Desktop/DesktopHeader.tsx

+  &:last-child {
+    &::after {
+      width: calc(100% + 40px);
+    }
+  }


comment;

해당 코드가 왜 필요한가 하고 살펴보니 MenuTitle에서 마지막 요소를 제외하고 padding-right를 줘서 메뉴간 간격을 주고 있기 때문이네요.

&:not(:last-child) { padding-right: 40px; }

not 선택자를 제거하고 MenuTitle에 padding-right: 40px를 주면 해당 코드를 작성해주지 않아도 될 것 같아요.

그런데 요소간 간격을 줄 때 padding-right를 주기 보다는 display: flex가 지정되어 있으니 gap을 활용해도 좋을 것 같습니다.

solar3070 · 2023-09-02T11:30:27Z

src/pages/recruit.tsx

-    }
-  `,
-};
+export { default } from '@src/views/RecruitPage';


question;

@src/views/RecruitPage/RecruitPage에 구현 사항 작성 후 @src/views/RecruitPage/index와 해당 파일에서 두 차례 re-export를 하는 것으로 파악했습니다.
해당 파일에 구현 사항을 직접적으로 작성하지 않고 이런 구조를 채택한 이유가 궁금합니다!

solar3070 · 2023-09-02T11:42:13Z

src/views/FAQPage/components/faq/QuestionBox.tsx

+    align-items: center;
    justify-content: space-between;
    /* align-items: center; */


suggestion;

79번째 줄과 겹치는 코드여서 주석을 지워도 괜찮을 거 같아요.

solar3070 · 2023-09-02T11:51:00Z

src/views/RecruitPage/RecruitPage.tsx

+import { ActivityReview } from './components/ActivityReview/ActivityReview';
+import ChapterInfo from './components/ChapterInfo';
+import Contact from './components/Contact';
+import FaqInfo from './components/FAQ';
+import NotificationSection from './components/NotificationSection';
+import RecruiteeInfo from './components/RecruteeInfo';
+import Schedule from './components/Schedule';


comment;

components 폴더 밑에 index.ts 파일을 만들어서 한번에 import 해도 좋을 것 같아요.
views의 다른 페이지 폴더들을 보니 그렇게 하는 곳도 있고 아닌 곳도 있네요.

solar3070 · 2023-09-02T12:31:05Z

src/views/RecruitPage/components/Contact/index.tsx

+const Contact = () => {
+  return (
+    <div>


suggestion;

div의 용도가 감싸주는 것이라면 Fragment를 사용해도 괜찮지 않을까요?

solar3070 · 2023-09-02T12:35:10Z

src/views/RecruitPage/components/FAQ/index.tsx

+  return (
+    <W>
+      <Wrapper>


question;

이 W는 Wrapper를 의미하는 건가요?
만일 맞다면 자식 컴포넌트의 Wrapper랑 겹치기도 하고, 의미 파악이 어려운 것 같아 좀 더 명확하게 바꾸는 게 좋을 것 같습니다!

solar3070 · 2023-09-02T13:01:23Z

src/views/RecruitPage/components/common/Tabs/index.tsx

+const allTabs: ExtraTabType[] = [
+  {
+    value: PartExtraType.ALL,
+    label: '전체',
+  },
+  {
+    value: Part.PLAN,
+    label: '기획',
+  },
+  {
+    value: Part.DESIGN,
+    label: '디자인',
+  },
+  {
+    value: Part.ANDROID,
+    label: '안드로이드',
+  },
+  {
+    value: Part.IOS,
+    label: 'iOS',
+  },
+  {
+    value: Part.WEB,
+    label: '웹',
+  },
+  {
+    value: Part.SERVER,
+    label: '서버',
+  },
+];


suggestion;

tabs 배열과 중복되는 요소가 많은 것 같은데 다음과 같이 작성해도 되지 않을까요?

Suggested change

const allTabs: ExtraTabType[] = [

{

value: PartExtraType.ALL,

label: '전체',

},

{

value: Part.PLAN,

label: '기획',

},

{

value: Part.DESIGN,

label: '디자인',

},

{

value: Part.ANDROID,

label: '안드로이드',

},

{

value: Part.IOS,

label: 'iOS',

},

{

value: Part.WEB,

label: '웹',

},

{

value: Part.SERVER,

label: '서버',

},

];

const allTabs: ExtraTabType[] = [

{

value: PartExtraType.ALL,

label: '전체',

},

...tabs,

];

SeojinSeojin · 2023-09-02T17:57:18Z

코드를 넓게넓게 봐주셨군요 .!! 값진 리뷰들 잘 받았습니다!!!

둘이 같은 컴포넌트를 쓰지 못하는 데에는 슬픈 사연이 있어요 ..
그것은 바로 .. 페이지별로 모바일/테블릿/데탑을 나누는 엔드포인트가 같지 않은데, 컴포넌트의 엔드포인트는 그 컴포넌트가 속한 페이지를 따라가다 보니, 그대로 옮겨 쓸 수가 없게 된 것입니다 🥲🥲

고치려면 어떻게 고쳐볼 수는 있지만, 그럼 PR 크기가 커지기도 하고 주 로직보다 다른 부분 변경사항이 더 많아질 것 같은 느낌이 듭니다 ..!!
따라서 다음에 요러한 주제에 대하여 이야기 나누어 보면 좋을 것 같아요!!

리팩토링 PR과 피쳐 PR을 분리할지에 대하여 (분리하면 깔끔해지긴 할텐데, 리뷰 속도에 따라서 작업 속도가 많이 느려질 수 있기도 해서 의견을 나누어 보고 싶어요!)
레거시, 어디까지 눈 감아 줄 것인가 ..?! (감지 말자👁️👁️는 의견도 환영이에용)
그 외 짱이 되는 방법 ..

f0rever0

안녕하세요! @solar3070 혜준언니의 코드리뷰 방식이 좋은 것 같아서 저도 suggestion; / comment; 방식에 따라서 코드리뷰를 진행해보았습니다!
이 pr만 보고 전체적인 코드 구조를 리뷰하는 건 어려울 것 같아서 중복되는 코드들에 대한 코멘트만 남겨보았습니다!! 공홈팀 화이팅✨

f0rever0 · 2023-09-03T02:11:02Z

src/views/RecruitPage/RecruitPage.tsx

+  /* 태블릿 뷰 */
+  @media (max-width: 1199px) and (min-width: 766px) {
+    margin-top: 90px;
+  }
+  /* 모바일 뷰 */
+  @media (max-width: 765.9px) {
+    margin-top: 90px;
+  }
+`;


comment;

데스크톱/태블릿/모바일 미디어 쿼리 코드를 독립된 파일로 관리해도 좋을 것 같아요~!

// mediaQuery.ts /* Desktop (해상도 1200px 이상)*/ const desktopQeury = () => `@media all and (min-width:1200px)`; /* 테블릿 가로, 테블릿 세로 (해상도 766px ~ 1199px)*/ const tabletQeury = () => `@media all and (min-width:766px) and (max-width:1199px)`; /* 모바일 가로, 모바일 세로 (해상도 765px 이하)*/ const mobileQeury = () => `@media all and (max-width:765px)`; export const media = { desktop: desktopQeury, tablet: tabletQeury, mobile: mobileQeury, };

${media.tablet} { margin-top: 90px; } ${media.mobile} { margin-top: 90px; }

f0rever0 · 2023-09-03T02:22:07Z

src/views/RecruitPage/components/NotificationSection/index.tsx

+  border: none;
+  background-color: transparent;
+  color: white;
+  font-family: SUIT;


suggestion;

styles/font.ts에 있는 font 속성들을 styles/global.ts에서 불러오던데 font-family: SUIT;와 같은 속성은 불필요하게 불러오는 것 같아요!

f0rever0 · 2023-09-04T01:04:39Z

src/views/RecruitPage/components/ActivityReview/ActivityReview.tsx

+                    {parsePartToKorean(review.part)}파트 {review.semester}기{'\n'}
+                    <DescName>{review.reviewer}</DescName>
+                  </Desc>
+                  <Arrow src={arrowRightWhite} alt="" />


question;

import로 arrowRightWhite만 ReactComponent로 불러오지 않은 이유가 있나요? alt 속성도 부여하면 좋을 것 같아요!

import { ReactComponent as ArrowLeft } from '@src/views/MainPage/assets/arrow-left-28x28.svg'; import { ReactComponent as ArrowRight } from '@src/views/MainPage/assets/arrow-right-28x28.svg'; import arrowRightWhite from '@src/views/MainPage/assets/arrow_right_white.svg';

f0rever0 · 2023-09-04T01:06:43Z

src/views/RecruitPage/components/ApplySection/index.tsx

+  return (
+    <Wrapper>
+      <Chip>YB RECRUITING</Chip>
+      <div style={{ height: '13px' }} />


question;

해당 줄에 inline-style을 사용한 이유가 있나요?

SeojinSeojin added 3 commits September 2, 2023 09:19

feat: add google sheet api

1834446

feat: add process.env reader

e1bebfa

feat : add Recruit Page UI

990ed04

pull-request-size bot added the size/XXL label Sep 2, 2023

SeojinSeojin requested review from f0rever0 and solar3070 September 2, 2023 00:27

fix: project description's white-space

42b002a

solar3070 reviewed Sep 2, 2023

View reviewed changes

f0rever0 approved these changes Sep 3, 2023

View reviewed changes

SeojinSeojin added 4 commits September 4, 2023 02:47

feat: recruit page 링크 변경

7617c17

fix color

4d4e9f7

fix: fix styles

1dcf057

fix: fix UI

f291360

f0rever0 approved these changes Sep 4, 2023

View reviewed changes

SeojinSeojin and others added 5 commits September 6, 2023 03:19

fix: apply design qa

18f117c

fix: fix padding of chips

33909e3

fix: fix uikit typo

5f21067

feat : 리크루팅 페이지 서버 자주 묻는 질문 내용 수정

a719eda

Merge pull request #158 from sopt-makers/fix/project-pre-wrap

d313cf2

SeojinSeojin changed the title ~~3기 SP-1 : Recruit 탭 추가~~ [SP0] Recruit 탭 추가 Sep 11, 2023

SeojinSeojin merged commit 81bb09a into develop Sep 11, 2023
1 check passed

SeojinSeojin deleted the feat/#153-recruit branch September 11, 2023 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SP0] Recruit 탭 추가 #157

[SP0] Recruit 탭 추가 #157

SeojinSeojin commented Sep 2, 2023

solar3070 left a comment

solar3070 Sep 2, 2023

solar3070 Sep 2, 2023

solar3070 Sep 2, 2023

solar3070 Sep 2, 2023

solar3070 Sep 2, 2023

solar3070 Sep 2, 2023

solar3070 Sep 2, 2023

SeojinSeojin commented Sep 2, 2023 •

edited

Loading

f0rever0 left a comment

f0rever0 Sep 3, 2023

f0rever0 Sep 3, 2023

f0rever0 Sep 4, 2023

f0rever0 Sep 4, 2023

[SP0] Recruit 탭 추가 #157

[SP0] Recruit 탭 추가 #157

Conversation

SeojinSeojin commented Sep 2, 2023

Summary

Screenshot

Comment

solar3070 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

SeojinSeojin commented Sep 2, 2023 • edited Loading

f0rever0 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

SeojinSeojin commented Sep 2, 2023 •

edited

Loading