refactor: migrate responsive Flex layouts - #189
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ef21db3 to
6b68131
Compare
|
현재 기준으로 마이그레이션 대상 140개 중 138개를 responsive Flex props로 전환해 약 98.6% 진행했습니다. 남은 2개는 Navigation의 모바일 코드 리뷰가 가능하도록 PR을 Ready 상태로 전환합니다. 다만 최종 병합은 |
|
@G-hoon 엇 영향이 왜 생길거라고 생각하신지는 약간 저도 궁금해요 리팩토링이 순차적으로 리뷰되어야 하는데 오히려 이 브랜치가 바로 base 를 향하면 이전 188의 커밋 히스토리가 여기에 같이 보이게 될 것 같은데.. 다른 회사에서는 이렇게 안하나 보군요 (저는 평소에 이렇게 작업을 하긴해서 궁금했습니다) |
|
아, 넵넵 저는 이 브랜치가 먼저 머지되고 그다음 188 머지되는 줄 착각했네요. 그리고 현재 브랜치가 빌드가 안되는 상황인 것 같습니다 ! |
|
@G-hoon |
G-hoon
left a comment
G-hoon
left a comment
There was a problem hiding this comment.
수고하셨습니다~!
큰 이슈는 없어 보여 Approve 합니다!
| import Navigation from './index'; | ||
|
|
||
| const { sendGAEvent } = vi.hoisted(() => ({ | ||
| sendGAEvent: vi.fn(), |
There was a problem hiding this comment.
이 부분 아래에 있는,
RecruitmentStatusSection/index.test.tsx 에서,
vi.mock('@next/third-parties/google', () => ({
sendGAEvent: vi.fn(),
}));
vi.mocked(sendGAEvent).mockClear();
쓰는 것처럼 통일해서 쓰는 건 어떨까요?
There was a problem hiding this comment.
말씀 주신 방식으로 통일하겠습니다. @next/third-parties/google에서 sendGAEvent를 import하고, mock에는 vi.fn()을 직접 선언한 뒤 vi.mocked(sendGAEvent).mockClear()로 초기화하도록 수정하겠습니다.
| ], | ||
| 'src/components/pages/Home/index.tsx': ["gap={{ sm: 0, md: '16px' }}"], | ||
| 'src/components/pages/People/index.tsx': ["gap={{ sm: '8px', md: '16px' }}"], | ||
| }; |
There was a problem hiding this comment.
responsiveFlexContracts와 allowedDeclarations 는
예를 들면 포맷팅이 바뀌거나 순서가 바뀌면, 깨지기 쉬울 것 같은데...
혹시 이 테스트가 꼭 필요한 게 아니라면 빼는 건 어떨까요?
There was a problem hiding this comment.
해당 테스트는 마이그레이션 과정에서 남은 Flex 선언과 responsive prop 전환 여부를 확인하기 위해 추가했습니다. 다만 말씀 주신 대로 소스 문자열과 선언 순서에 의존해 포맷팅 변경에도 깨질 수 있어 유지 비용이 크다고 판단했습니다. 기존 컴포넌트 렌더링 테스트를 유지하고, 이 contract 테스트는 제거하겠습니다.
| })); | ||
|
|
||
| vi.mock('@/libs/assets/icons', () => ({ | ||
| CheckCircleIcon: () => <svg aria-label="지원 조건 충족" />, |
There was a problem hiding this comment.
There was a problem hiding this comment.
이 부분도 다른 테스트와 동일하게 ReactNode를 type import해서 사용하도록 정리하겠습니다.
2 participants
작업 내용
@sipe-team/side의 responsive props로 마이그레이션했습니다.direction,align,justify,gap반응형 값을 컴포넌트 props로 이동했습니다.ActiveCard는Flex asChild를 사용해 기존 외부 링크의 anchor 구조를 유지했습니다.진행 현황
gap과 Flex item의flex,align-self14개는 대상에서 제외현재
FlexAPI로 안전하게 표현할 수 있는 대상은 모두 전환했습니다.유지한 선언
Navigation의
.headerLayout에 있는display: flex와justify-content: space-between은 SCSS에 유지했습니다.해당 레이아웃은 모바일에서
display: block, 데스크톱에서display: flex로 변경됩니다. 현재 responsive Flex API는 breakpoint별display변경을 지원하지 않아 이를 props로 옮기면 모바일 레이아웃 동작이 달라집니다. 추가 wrapper를 만들거나 공용Layout컴포넌트의 계약을 변경하는 것보다 기존 구조를 유지하는 편이 안전하다고 판단했습니다.나머지 14개 선언은 CSS Grid의 간격 또는 Flex item 자체의 크기와 정렬을 결정하는 속성으로, Flex container props의 마이그레이션 대상이 아니므로 기존 SCSS에 유지했습니다.
배포 및 병합 조건
현재 PR에는 responsive Flex 마이그레이션 코드를 미리 반영해 두었습니다.
side의 responsive Flex 변경이 병합되고 패키지가 배포되면 이 PR에서@sipe-team/side버전과 lockfile을 갱신합니다. 이후 전체 테스트, 타입 검사, production build, 배포 Preview를 다시 확인하고 최종 병합할 예정입니다.테스트