-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FE] refactor: 랜딩 페이지 리팩토링 및 서비스 소개 section 제작 #371
Conversation
…eview-me into fe/refactor/354-new-landing-page
Co-authored-by: badahertz52 <[email protected]>
top: calc((100% - 1.6rem) / 2); | ||
right: 1rem; | ||
|
||
width: 1.6rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추후에 width, height를 상수로 빼도 좋을 듯!
justify-content: center; | ||
gap: 2rem; | ||
|
||
background-color: white; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
background-color 에서 theme에 있는 white를 사용하는 것은 어떨까요?
@@ -74,7 +73,7 @@ const ReviewAccessForm = () => { | |||
/> | |||
<Button | |||
type="button" | |||
styleType={isValidAccessCodeInput(groupAccessCode) ? 'primary' : 'disabled'} | |||
// styleType={isValidAccessCodeInput(groupAccessCode) ? 'primary' : 'disabled'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분을 주석처리한 이유가 뭘까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마 groupAccessCode를 사용하지 않으니 제거하고 무언의 테스트를 해보려다가 다시 살린 것 같네요..ㅋㅋㅋ 이제 사용하지 않는 컴포넌트라 아예 삭제했습니다.
…eview-me into fe/refactor/354-new-landing-page
/> | ||
|
||
<OverviewItem | ||
direction="column" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OvervieItem 이라는 멋진 컴포넌트를 만들었내요.
추후에 리팩토링을 한다면, OverviewItem 에 사용하는 내용들을 객체로 관리하면 좋을 듯해요.
@@ -74,7 +73,7 @@ const ReviewAccessForm = () => { | |||
/> | |||
<Button | |||
type="button" | |||
styleType={isValidAccessCodeInput(groupAccessCode) ? 'primary' : 'disabled'} | |||
// styleType={isValidAccessCodeInput(groupAccessCode) ? 'primary' : 'disabled'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석 지워주면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하지 않는 컴포넌트라 아예 삭제했습니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 홈 페이지에서 세로로 스크롤이 생기는데, 처음 화면에 들어왔을 때 스크롤이 생기는 것 보다는 이미지와 글씨가 작더라도 한 화면에 다 나오는 게 좋다고 생각하는데 올리는 어떻게 생각하시나요???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
올리 페이지 작성하느라 고생했어요
코멘트 남겼지만, 아직은 문제될 사항은 아니라서 approve할게요
onFocus,onBlur 시 에레메세지는 데모데이안에 구현해야할 것 같아서 이 부분은 월요일 프론트회의에서 다뤄봐요
description: string[]; | ||
} | ||
|
||
const OverviewItem: React.FC<OverviewItemProps> = ({ direction, imageSrc, title, description }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const OverviewItem: React.FC<OverviewItemProps> = ({ direction, imageSrc, title, description }) => { | |
const OverviewItem: = ({ direction, imageSrc, title, description }:OverviewItemProps) => { |
이라고 하지 않고 React.FC<OverviewItemProps>
을 사용한 이유가 있나요?
|
||
const handleCloseButtonClick = () => { | ||
if (isChecked) closeModal(); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return 을 한 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FC랑 쓸데없는 리턴 지웠어요~!
<S.WarningWrapper> | ||
<S.CheckContainer> | ||
<Checkbox | ||
id="is-confirmed-checkbox" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is-confrimed-checkbox
id는 어디서 사용하는 건가요?
id는 유일한 값이여아하는데 'is-confrimed-checkbox' 는 어디선가 또 쓸 수 있어서 useId를 사용해보는 것도 좋을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id를 따로 사용하는 곳은 없고 checkbox에서 id를 필수로 받고 있어서 기입했던 것 같아요
useId는 무엇일까요?!
confirm: 'CONFIRM', | ||
}; | ||
|
||
const URLGeneratorForm = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input개수 만큼 state가 있고 URLGeneratorForm 의 태그가 현재는 section이네요
리뷰생성을 위한 데이터를 하나의 state에서 관리할 수 있는 방법도 있고 input 개수 만큼의 state로 관리하는 방법이 있는데
각 방법의 장단점이 있죠
이 부분이 어떻게 리팩토링 될지 (좋은 방향으로) 기대됩니다! 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URLGeneratorForm
은 section이고, 그 자식 컴포넌트인 FormLayout
이 form인 상태입니다.
예전 레이아웃에서는 URL 생성, 리뷰 확인의 폼 배치가 달라서 FormLayout
에 폼 정렬 속성(row, col)을 받을 수 있는 컴포넌트로 특화시키고, 그 FormLayout
을 사용하는 최상위 컴포넌트(Container 느낌)로 URLGeneratorForm
을 뒀었는데 폼이 하나만 있는 지금은 불필요한 코드일 수 있겠다는 생각이 들었어요.
하지만 일단은 업보로 달아두는 것으로..,😶
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
랜딩 페이지 한층 업그레이드~ 됐네요! 수고했어요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
올리 수고했어요!
margin-bottom: 3rem; | ||
`; | ||
|
||
export const OverviewTitle = styled.h2` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저번에 바다가 말해줬던 것 처럼 현재 h1
태그가 사용되고 있지 않아서 h2
보다는 span
으로 변경해주시면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타이틀은 제목인 만큼 줄바꿈 및 강조가 있어야 한다고 생각해서 p
태그로 수정했습니다~~
<S.WarningWrapper> | ||
<S.CheckContainer> | ||
<Checkbox | ||
id="is-confirmed-checkbox" | ||
isChecked={isChecked} | ||
handleChange={handleCheckboxClick} | ||
$style={{ width: '2.3rem', height: '2.3rem' }} | ||
/> | ||
<S.CheckMessage>URL을 저장해두었어요!</S.CheckMessage> | ||
</S.CheckContainer> | ||
<S.Warning>* 창이 닫히면 URL을 다시 확인할 수 없어요!</S.Warning> | ||
</S.WarningWrapper> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드컨벤션보면 Wrapper
가 하나의 요소를 감쌀 때 사용되고, Container
가 여러 개의 요소를 감쌀 때 사용된다고 적혀있어요. Wrapper
가 Container
보다 상위에 있어서 조금 어색한 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반영했습니다~
export const HomePage = styled.div` | ||
display: flex; | ||
min-height: 100vh; | ||
width: 100vw; | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 첫 화면 들어왔을 때 세로 스크롤이 생기는 이유 중 하나가 min-hiehgt: 100vh
인 것 같다는 느낌이? 살짝 드네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩토링 초기에는 min-hiehgt: 100vh
가 없으면 보라색 배경이 화면 끝까지 닿지 못하고 잘리는 문제가 있었는데, 지금 코드에서는 없애도 잘 동작하네요~
하지만 스크롤바가 생기는 이유는 해당 section 안의 요소 크기 때문인거라 이 요소들의 크기를 줄여야 하는데 스크롤바가 없을 정도로 줄이면 안내의 의미가 없어질 정도로 작아져야 해서 문제입니다 😢
const getCompleteURL = (reviewRequestCode: string) => { | ||
return `${window.location.origin}/user/review-writing/${reviewRequestCode}`; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 작성 경로를 constants/routes.ts
에 정의되어 있는 상수들을 활용해보는 건 어떨까요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수 도입 전에 작성한 코드라 하드코딩이 남아있네요 수정할게요~~
export { default as LandingPage } from './LandingPage'; | ||
export { default as HomePage } from './HomePage'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와우! 랜딩에서 홈으로 바로 변경해주셨네요👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰만 달아놓고 Finish를 안 했었네요😅 고생했어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input 유효성 검사 테스트까지 진행하셨네요👍
}; | ||
|
||
const getCompleteURL = (reviewRequestCode: string) => { | ||
return `${window.location.origin}/user/review-writing/${reviewRequestCode}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이제 url이 리뷰 작성 페이지가 아니라 리뷰 연결 페이지로 이동하게 해서 수정해야겠어요.!
🚀 어떤 기능을 구현했나요 ?
URLGeneratorForm
을 리팩토링했습니다.Input
추가ReviewAccessCode
를 사용하는 부분을 제거했습니다.Landing
에서Home
으로 변경했습니다.Section
을 만들었습니다.EyeButton
컴포넌트를 만들었습니다.🔥 어떻게 해결했나요 ?
input
이 0자이므로 길이 에러가 뜬다는 문제점이 있었습니다.onFocus
와onBlur
관련 이벤트를 처리해야 하지만 현재 시급한 부분은 아니라고 생각해서 다음과 같이 코드를 작성했습니다.textarea
처럼 별도의 훅으로 관리할 예정입니다.📝 어떤 부분에 집중해서 리뷰해야 할까요?
sidebarBackground
로 나와있는데, 실제로 이 색상을 적용하면 피그마와 달리 어둡게 나옵니다. 아마 투명도가 적용돼서 그런 것 같습니다.css는 적당히 깎았는데 크게 거슬리는 부분이 있다면 말씀해주세용ㄴ 얘기 나왔던 대로 서비스 소개 Section은 임시로 놔두기로 했습니다. 현 디자인상 한 화면에 다 들어오게 하려면 글씨가 너무 작아지는 문제가 있습니다
📚 참고 자료, 할 말