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

[TEAM3] Petit Project 개인 프로필 화면 구현 #19

Open
wants to merge 3 commits into
base: petit-project/team3
Choose a base branch
from

Conversation

wooogi123
Copy link

No description provided.

@wooogi123 wooogi123 self-assigned this Jun 18, 2022
Copy link
Member

@dev-owen dev-owen left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

import { useMediaQuery, useMediaValue } from './hooks';

const Akalee: React.FC = () => {
globalStyles();
Copy link
Member

Choose a reason for hiding this comment

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

stitiches 라이브러리에서 전역 css를 이렇게 처리하는 군요! 저는 일반적으로 global.css 파일에 전역 css 파일을 봐 왔었어서 신기합니다 :)

return (
<AspectRatioBox style={{ width: size, paddingTop: size }}>
<ClockBox
style={{
Copy link
Member

Choose a reason for hiding this comment

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

이 부분은 관심사 분리 측면에서 css 속성에 대한 내용은 하위 컴포넌트(View 역할 하는 컴포넌트)에서만 처리해주게 수정을 해 주어도 좋을 것 같습니다 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

상위 이벤트의 값을 하위 component로 넘겨줄 경우 컴포넌트의 recomposition 비용이 크다 판단해서 react 렌더링과 연관 없는 css variables을 사용해서 진행하였는데, 보다 좋은 방법이 있을까요?


export default ClockCoreFrame;

const TIME_PIVOT_ITEMS = [
Copy link
Member

Choose a reason for hiding this comment

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

상수 값들은 constants.ts 등의 파일로 별도로 빼도 좋을 것 같아요

{ text: 'XII', deg: 360 },
] as const;

const Box = styled('div', {
Copy link
Member

Choose a reason for hiding this comment

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

styles 값들은 styles.ts 로 별도로 빼도 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

요 부분은 스타일 차이가 많이 갈릴 것 같은데요, 저는 개인적으로 해당 컴포넌트에서 사용하기 위한 style 정의 또한 컴포넌트 정의 파일의 context에 묶여있다 생각해서 파일을 분리하는 것 보다 하단으로 분리하는게 명시적이라 생각합니다. 어떻게 생각하시나요?

}

const ClockHand: React.FC<ClockHandProps> = ({
time,
Copy link
Member

Choose a reason for hiding this comment

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

time 이라는 값으로 시, 분, 초를 받아오다보니 조금 혼란스러울 수도 있을 것 같아요. type: 'hour' | 'minute' | 'second' 와 같이 타입을 추가해 주거나, 아니면 optional한 속성으로 hours?: number, minutes?: number .. 이런 식으로 구분 주어도 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

해당 컴포넌트의 경우 time, min, max 값을 기준으로 단순히 360 deg에서의 포지션을 잡아주는 렌더링만 담당하고 있어, 불필요한 타입 구분이 필요 없다 판단했습니다.

.mockImplementation((cb) => {
setTimeout(() => {
cb(delta);
delta += 16;
Copy link
Member

Choose a reason for hiding this comment

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

16ms씩 추가를 하는 이유가 혹시 어떤 것인지 알 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

useClockState 훅 내부에서 raf를 사용한 타이머를 통해 시간을 계산하고 있어, 해당 web api에 대한 polyfill 목적입니다!

@wooogi123 wooogi123 requested a review from dev-owen June 21, 2022 02:13
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.

2 participants