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

[Feat] class 상세 내 탭 뷰 구현 #82

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

hansoojeongsj
Copy link
Collaborator

@hansoojeongsj hansoojeongsj commented Jan 15, 2025

📌 Related Issues

✅ 체크 리스트

  • PR 제목의 형식을 잘 작성했나요? e.g. [Feat] PR 템플릿 작성
  • 빌드가 성공했나요? (pnpm build)
  • 컨벤션을 지켰나요?
  • 이슈는 등록했나요?
  • 리뷰어와 라벨을 지정했나요?

📄 Tasks

클래스 상세 페이지 내 탭 뷰 퍼블리싱
아이콘은 icClose로 대체했습니다.

⭐ PR Point (To Reviewer)

  • 줄바꿈
  • 날짜..?

📷 Screenshot

스크린샷 2025-01-16 061729
스크린샷 2025-01-16 061738
스크린샷 2025-01-16 061744
스크린샷 2025-01-16 061749

🔔 ETC

새롭게 알게 된 내용 !!
package.json에 scripts에
"dev": "vite--host",
-> 핸드폰에서 확인 가능

@hansoojeongsj hansoojeongsj added 🛠️ Feature 기능 개발 수정 첫 사랑니 labels Jan 15, 2025
@hansoojeongsj hansoojeongsj self-assigned this Jan 15, 2025
@hansoojeongsj hansoojeongsj linked an issue Jan 15, 2025 that may be closed by this pull request
1 task
Copy link
Collaborator

@heesunee heesunee left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 ㅎㅎ 코멘트 남긴 부분 한번 확인하고 반영해주세요!

@@ -0,0 +1,4 @@
import * as React from "react";
Copy link
Collaborator

Choose a reason for hiding this comment

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

요거 지워주세요!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

목업 데이터 네임 형식 맞추면 좋을 것 같아요 ! mockLessonData로 파일이름이랑 데이터 이름 변경해주세요 !!

Comment on lines 6 to 8
interface LessonData {
lessonDetail: string;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

형식 맞춰주세요 ! LessonDataProps ~~

Comment on lines 4 to 12
export const roundBox = style({
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
backgroundColor: vars.colors.main04,
borderRadius: '4px',
marginRight: '1rem',
padding: '0.6rem 1.2rem',
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

roundBoxStyle로 이름 변경해주세요!

Comment on lines +10 to +41
// 시간을 계산, "익일" 여부를 판단
const calculatePeriod = (start: string, end: string) => {
const startDate = new Date(start);
const endDate = new Date(end);

const startTime = `${startDate.getHours().toString().padStart(2, '0')}:${startDate
.getMinutes()
.toString()
.padStart(2, '0')}`;
const endTime = `${endDate.getHours().toString().padStart(2, '0')}:${endDate
.getMinutes()
.toString()
.padStart(2, '0')}`;

const isNextDay = endDate.getDate() !== startDate.getDate();
const formattedEndTime = isNextDay ? `익일 ${endTime}` : endTime;

const totalMinutes = Math.abs(endDate.getTime() - startDate.getTime()) / 1000 / 60;
const hours = Math.floor(totalMinutes / 60);
const minutes = totalMinutes % 60;

const durationString = minutes > 0 ? `총 ${hours}시간 ${minutes}분` : `총 ${hours}시간`;

return { startTime, formattedEndTime, durationString };
};

// 날짜를 포맷팅 (ex. "2025년 1월 8일 수요일")
const formatDate = (dateString: string) => {
const date = new Date(dateString);
const days = ['일요일', '월요일', '화요일', '수요일', '목요일', '금요일', '토요일'];
return `${date.getFullYear()}${date.getMonth() + 1}${date.getDate()}${days[date.getDay()]}`;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

시간 변환해서 사용하는 페이지가 많은 것 같은데, utils 폴더에 시간 관련 함수 파일 하나 만들어서 분리해주시면 좋을 것 같아요! 제 페이지에서도 사용되는 똑같은 로직이 있어서 !!

Comment on lines 4 to 8
export const TabPanelStyle = style({
padding: '2.4rem 2rem',
borderTop: '1px solid',
borderColor: vars.colors.gray01,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

style 이름 시작 소문자로 해주세요! tabPanelStyle

Comment on lines 4 to 14
export const cardStyle = style({
width: '100%',
display: 'inline-flex',
padding: '1.6rem 2rem',
alignItems: 'center',
borderRadius: '4px',
border: '0.5px solid',
borderColor: vars.colors.gray02,
backgroundColor: vars.colors.white,
gap: '0.8rem',
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 border랑 borderColor 따로 되어있는데, border: 0.5px solid ${vars.colors.gray02} 로 한번에 적어도 될 것 같아요!

Copy link
Member

@KIMGEONHWI KIMGEONHWI left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!! 코드리뷰 남긴 부분 한번만 확인해주세요.

@@ -0,0 +1,34 @@
export const lessonData = {
Copy link
Member

Choose a reason for hiding this comment

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

저희 상수파일 네이밍은 BIG_SNAKE_CASE 방식을 사용하기로 해서, LESSON_DATA로 수정해주세요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다! 추가로 이후 API 개발 이후에 서버 데이터 활용하는 constants는 constants폴더 밑에 mocks 폴더를 두고 따로 관리해도 좋을 것 같다고 생각합니다!


return (
<>
<Flex padding="0.8rem 0 0">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Flex padding="0.8rem 0 0">
<Flex paddingTop="0.8rem">

으로 사용하는 것이 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 padding top 넣기 전에 했던 거라 수정해두겠습니다..!!

Copy link
Member

Choose a reason for hiding this comment

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

BottomWrapper보다는 Tab에 관련된 컴포넌트들이 담기기 때문에 TabWrapper를 사용하는 것이 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 ~ 넵

</TabList>
</Flex>

<div className={TabPanelStyle}>
Copy link
Member

Choose a reason for hiding this comment

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

div에 스타일을 적용시켜주기보다는 Flex컴포넌트를 사용하는 것이 어떨까요?

Copy link
Collaborator

@rtttr1 rtttr1 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~!!

return <div>수업 정보 상세</div>;
return(
<>
<BottomWrapper colorScheme="primary" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

프래그먼트 빼도 될거같아요!

Copy link
Collaborator

@constantly-dev constantly-dev left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!! 고려할 것이 많은 view인데 잘 구현해주신 것 같아요~

@@ -0,0 +1,34 @@
export const lessonData = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다! 추가로 이후 API 개발 이후에 서버 데이터 활용하는 constants는 constants폴더 밑에 mocks 폴더를 두고 따로 관리해도 좋을 것 같다고 생각합니다!

Comment on lines 6 to 11
interface LessonDataProps {
lessonDetail: string;
}

const Intro = () => {
const { lessonDetail }:LessonDataProps = lessonData;
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 lessonData(상수)는 타입 추론이 되기 때문에 따로 타입 지정을 해주지 않아도 될 것 같습니다!
image

Comment on lines 9 to 16
interface LessonDataProps {
lessonLevel: string;
lessonLevelDetail: string;
lessonRecommendation: string;
}

const Level = () => {
const { lessonLevel, lessonLevelDetail, lessonRecommendation }: LessonDataProps = lessonData;
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 부분도 다른 코드 리뷰에서 작성한 것처럼 타입 추론이 되기 때문에 타입 지정을 안 해도 될 것 같습니다!! 👍

Comment on lines +17 to +22
{lessonDetail.split('\n').map((line, idx) => (
<React.Fragment key={idx}>
{line}
<br />
</React.Fragment>
))}
Copy link
Collaborator

Choose a reason for hiding this comment

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

물론 해당 데이터는 이후 서버에서 받을 데이터이긴 하지만 만약 서버에서 string 문자열을 줄 때 공백 문자를 포함해서 주고 그걸 클라이언트에서 줄바꿈 처리를 해야 한다면 해당 로직이 아닌 whiteSpace: 'pre-line' 속성을 쓰면 될 것 같습니다!
whiteSpace: 'pre-line'는 줄바꿈 문자를 처리 해주기 때문에 split으로 확인하지 않고도 가능할 것 같습니다!

Comment on lines 47 to 58
<TabPanel isSelected={selectedTab === 0}>
<Intro />
</TabPanel>
<TabPanel isSelected={selectedTab === 1}>
<Level />
</TabPanel>
<TabPanel isSelected={selectedTab === 2}>
<Period />
</TabPanel>
<TabPanel isSelected={selectedTab === 3}>
<LocationInfo />
</TabPanel>
Copy link
Collaborator

Choose a reason for hiding this comment

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

물론 지금 코드도 잘못된 것은 아니지만 확장성을 고려해서 필요한 데이터를 상수화하고 map을 돌리는 것도 좋은 방법이 될 것 같습니다! 그렇게 되면 상수(배열 형식)에 데이터를 추가만 하면 확장이 편하게 되기 때문에 해당 설계도 고려해보면 좋을 것 같습니다!

예시

export const THEATER_TABS = [

  {
    id: 1,
    name: '일반관',
  },
  {
    id: 2,
    name: '스페셜관',
  },

];
{THEATER_TABS.map(({ id, name }) => (
		<S.Tab key={id} $isActive={activeTab === id} onClick={() => handleTabClick(id)}>
			{name}
		</S.Tab>
))}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
수정 첫 사랑니 🛠️ Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] Class 상세 중 Tab 퍼블리싱
5 participants