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

[ team12 ] 더미데이터 기반 UI, TypeScript로 Calendar library 제작 #25

Merged
merged 13 commits into from
May 26, 2021

Conversation

jjunyjjuny
Copy link
Collaborator

@jjunyjjuny jjunyjjuny commented May 25, 2021

UI

  • 더미 데이터를 기반으로 1차 적인 UI만 구성해놓았습니다.
  • 아직 API가 오지 않아서 구체적인 UI는 API가 나온 후 다시 진행할 예정입니다.

Calendar

  • TypeScript로 범용성 있는 라이브러리 제작 진행중입니다.

중점적으로 리뷰 받고자 하는 부분

TypeScript

  • src/lib 내에 있는 TypeScript 로 작성한 캘린더 라이브러리 기준으로 리뷰해주시면 감사하겠습니다!

styled-components

  • src/lib/Carousel.tsx에서 "Slider" styled-component만 Carousel 내에 선언을 했는데, 다른 styled-component처럼 전역에 선언해두면 애니메이션이 최초 1회만 실행되기 때문입니다.
  • 원인은 Carousel이 리렌더 되더라도 Slider가 리렌더되는 조건이 충족되지 않아서 (전달받는 props도 변하지 않고, 전역에 있는 Slider라서 메모리주소가 유지되기 때문에??) 애니메이션이 끝났던 DOM을 재활용하는 것 같습니다.
  • 이건 React.memo의 기능같은데 styled-component가 자체적으로 캐싱을 하는건지 모르겠습니다. 저는 부모가 렌더되면 자식도 아예 새로 렌더되는거로 알고 있었는데, 캐싱을 하더라구요. 그래서 animation을 사용하는 styled-component만 Carousel 내부에 선언해서 Carousel이 리렌더(함수 재실행) 될 때 마다 새로 선언하게 해뒀는데. 이런 패턴이 맞을지 모르겠습니다.
  • 나중에 규모가 더 큰 프로젝트에서는 모듈로 따로 관리할거 같은데.. 이러면 Carousel 메인 로직과 return 사이에 styled-component가 끼어들어 가독성을 해칠거 같아요

표현식 vs 선언문

  • 컴포넌트 함수를 선언문, 표현식으로 선언하는 것에 대해 생각보다 이슈?가 있는거 같던데 현업에서는 별로 신경 안 쓰는거 같기도 하고.. JS할 때는 표현식을 썼는데 React+TypeScript의 경우 선언문을 쓴다는 얘기가 있더라구요
  • 글 써본것
  • 선언문 vs 표현식은 사실 취향의 차이가 더 클거라고 생각했는데 표현식에 한계가 있다는 글이 있더라구요. => 이건 제가 번역해본 것
  • React.FC가 Generics를 허용하지 않아서 라이브러리 같은거 만들 때 문제가 있다는데..흠..

jjunyjjuny and others added 13 commits May 19, 2021 00:14
헤더를 제외한 홈페이지 마크업 진행
달력, 차트 라이브러리 ts로 하기 전 필요한 마크업들 1차로 완료
이것저것 해보느라 커밋 시점을 잡지 못해서 저장하기 위해..
스타일 외 메인로직 구현
@jjunyjjuny jjunyjjuny requested a review from crongro May 25, 2021 02:13
GangWoon pushed a commit that referenced this pull request May 26, 2021
- ViewController에서 보관하던 정보, 로직 전부 ViewModel로 이동
- ViewModel 역할에 따라 공용 인터페이스 구축

Dae-Hwa/airbnb/#25
GangWoon pushed a commit that referenced this pull request May 26, 2021
- 쓰지 않는 메소드, 중복 삭제

Dae-Hwa/airbnb/#25
GangWoon pushed a commit that referenced this pull request May 26, 2021
- Network Manager에서 다루도록 변경

Dae-Hwa/airbnb/#25
GangWoon pushed a commit that referenced this pull request May 26, 2021
GangWoon pushed a commit that referenced this pull request May 26, 2021
GangWoon pushed a commit that referenced this pull request May 26, 2021
- PopularLocation 데이터 받아오기 성공한 경우
- 받아오지 못하고 400 오류가 뜬 경우

Dae-Hwa/airbnb/#25
@crongro
Copy link
Contributor

crongro commented May 26, 2021

  • 원인은 Carousel이 리렌더 되더라도 Slider가 리렌더되는 조건이 충족되지 않아서 (전달받는 props도 변하지 않고, 전역에 있는 Slider라서 메모리주소가 유지되기 때문에??) 애니메이션이 끝났던 DOM을 재활용하는 것 같습니다.

이 부분은 animation & keyframe 속성 때문인거 같네요.
리렌더링 될때, 전달되는 값이 달라도 안되는거죠? 그건 좀 이상하지만, 현상이 그러하니. 예상한것처럼 DOM에 붙어 있는 animation이 추가로 셋팅되지 않는거 같네요. 그렇다고 이게 react의 문제인가? 싶기도하네요. 어떤 상황에서 바닐라에서도 비슷한 현상이 나올지도요.

리렌더링 되기전에 animation 속성을 제거해주는 편법이 통하려나요? ㅎ

보통 애니메이션 컨트롤은 transition을 추가/제거 하는 방법을 사용하긴하죠.
더 디테일한 애니메이션 조작은 animationframe으로 제어를 할 수도 있겠고요.

@crongro crongro self-assigned this May 26, 2021
@crongro
Copy link
Contributor

crongro commented May 26, 2021

  • 컴포넌트 함수를 선언문, 표현식으로 선언하는 것에 대해 생각보다 이슈?가 있는거 같던데 현업에서는 별로 신경 안 쓰는거 같기도 하고.. JS할 때는 표현식을 썼는데 React+TypeScript의 경우 선언문을 쓴다는 얘기가 있더라구요

함수표현식은 람다표기법으로 간단한 함수를 표현할때 좋은거 같고요.
나머지는 함수선언문이 호이스팅의 이유등으로 더 좋은거 같네요.
함수선언문에 TS적용시 좀더 자연스러워 보이는 기분도 들더군요.

React.FC는 안티패턴처럼 언급되는 경우가 많은거 같아요. type, interface 만들어서 일반 함수처럼 TS 적용하는 것을 더 추천하는 경우도 많더군요. 참고하신 글등의 이유가 그 중 하나인거 같아요.

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

잘 구현했어요. 라이브러리화 시키는 시도 좋고요.
많이 쓰이면서 개선될테니 발전시켜나가면 좋겠네요. 이제 시작이라고 생각하시고.

파일 사이즈가 커지면 부분적으로 파일을 나누는 시도해보셔도 될 듯.
컴포넌트가 두 개씩 있다거나, 아니면 규모가 있는 이런저런것이 있다면 과감하게 분리하셔도 될 거 같아요.
지금 수준보다 코드가 커지면 더욱 고려해보시고요.

참. 블로그 글 자주 쓰는 거 같아서 아주 좋아보입니다.

import CalendarProvider, { useCalendarState } from "./CalendarProvider";
import { Direction, OnClickDay, _OnClickResult } from "./type";

// type이 좋을까 interface가 좋을까
Copy link
Contributor

Choose a reason for hiding this comment

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

대부분 interface로 정의하는것이 전 좋아보임.
type은 기존 타입을 조합해서 새로운 타입으로 만드는 용도로 좀더 어울리는 거 같네요.


export default function Calendar({ onClickDay, countOfMonth }: CalendarProps) {
if (countOfMonth < 1 || countOfMonth > 12) {
throw new Error(MESSAGE.ERROR.INVAILD_RANGE_COUNT_OF_MONTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

message 분리 좋네요.

Copy link
Contributor

Choose a reason for hiding this comment

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

그런데 에러가 사용자에게 전달되야 하는 것이면, 실제로 화면에는 어떻게 처리되는지도 나중에는 고민해보세요.


export type OnClickResult = _OnClickResult;

function _Calendar({ countOfMonth }: { countOfMonth: number }) {
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 +28 to +29
const state = useCalendarState();
const { calendarList } = state;
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 +38 to +44
<li>일</li>
<li>월</li>
<li>화</li>
<li>수</li>
<li>목</li>
<li>금</li>
<li>토</li>
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 +27 to +30
function createDays(day: number): number[] {
const days = Array.from({ length: day }, (_, i) => i + 1);
return days;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

이 함수는 렌더링될때마다 호출될까요?
그렇다면 매번 같은 결과를 만드는 경우도 있을까요?

문제가 발견되면 개선도 해보셔요~

<DayWrapper>
<DayCircle
onClick={() => {
if (!onClickDay) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서도 optional chaining 문법이 가능할까요?

import { MESSAGE } from "./constant";
import { Calendar, OnClickDay } from "./type";

export default function CalendarProvider({
Copy link
Contributor

Choose a reason for hiding this comment

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

스스로의 컨벤션이 있으신것 같은데, 그냥 이건 취향문제인데요.

type, reducer 코드가 위쪽으로 오고,
컴포넌트 코드 중간에 자리잡고.
스타일은 맨 뒤로 가도 되고,

reducer의 경우 다른 파일로 분리하는 것도 좋고요. 커스텀 훅도 가능할테고.

그런 순서도 생각해보세요.

const CalenderDispatchContext = createContext<Dispatch<Action> | null>(null);
const CalenderMethodContext = createContext<Method | null>(null);

function reducer(state: State, action: Action): State {
Copy link
Contributor

Choose a reason for hiding this comment

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

reducer 가 추가될 수도 있으니, 구체적인 이름으로 변경해보세요.

Comment on lines +73 to +86
case "MOVE_RIGHT":
newCalendarList.shift();
newCalendarList.push(createCalendar(year, month + countOfMonth + 1));

const nextDate = new Date(year, month + 1);
newCurrentDay.year = nextDate.getFullYear();
newCurrentDay.month = nextDate.getMonth();

return {
...state,
calendarList: newCalendarList,
currentDay: newCurrentDay,
x: action.x,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

중복 코드 일부라도 함수로 분리해서 중복을 제거해볼까요?

@crongro crongro merged commit 78fb3a4 into codesquad-members-2021:team12 May 26, 2021
crongro pushed a commit that referenced this pull request May 27, 2021
Feat. useAxios 작업: api요청하여 요금데이터 받아옴
ksundong pushed a commit that referenced this pull request May 27, 2021
전체 숙소 목록 보기 & 숙소 등록 기능 추가
crongro pushed a commit that referenced this pull request Jun 1, 2021
crongro pushed a commit that referenced this pull request Jun 2, 2021
crongro pushed a commit that referenced this pull request Jun 2, 2021
feat: 달력 UI 구현 중  & 폴더구조 변경 (#25)
crongro pushed a commit that referenced this pull request Jun 2, 2021
crongro pushed a commit that referenced this pull request Jun 2, 2021
crongro pushed a commit that referenced this pull request Jun 2, 2021
crongro pushed a commit that referenced this pull request Jun 8, 2021
[FE] 검색 fetch 요청, 응답으로 렌더링, 지도 api 사용, searchBar 요금 fetch 요청
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants