-
Notifications
You must be signed in to change notification settings - Fork 1
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 ] 검색 필터 모달 퍼블리싱 #25
Conversation
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.
크게 수정해야할 부분은 없어 보이네요! 고생하셨습니다.
구두로 이야기 했던 것 처럼, 오키나와를 제외한 다른 도시를 클릭하여도 검색하기
버튼이 활성화는 되지만 모달이 닫히지는 않도록 해주세요! 모달이 닫히는 이벤트는 오키나와
가 선택되었을 때만 가능하게요.
<> | ||
<PriceModal isVisible={isModalOpen} onModalClose={onModalToggle} /> | ||
</> |
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.
p5: 빈태그 삭제해도 괜찮을거 같아요
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.
맞숩니다,,
src/pages/home/cityGroups.ts
Outdated
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.
p5: 상수데이터는 src/constants 폴더 생성해서 해당 폴더에 위치할 수 있도록 해주세요!
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.
넵 수정 커밋 날렸습니다!
src/components/home/SearchModal.tsx
Outdated
const onOkinawaClicked = (state: boolean) => { | ||
setIsButtonClicked(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.
p2: 해당 이벤트 핸들러 네이밍을 onOkinawaClicked
보다는 네이밍 컨벤션에 맞추어 handleOkinawaClick
을 사용하는 것이 어떨까요? 또한, 클릭 시 상태가 직곽적이지 못한 것 같아. 다음과 같이 수정하는 것은 어떨까요?
const onOkinawaClicked = (state: boolean) => { | |
setIsButtonClicked(state); | |
}; | |
const handleOkinawaClick = () => { | |
setIsButtonClicked((prev) => !prev); | |
}; |
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.
오 좋습니다 !!
요거 함수 onClick 으로 아예 변경 될 예정으로 로직이 아예 변경되었어유
> | ||
검색하기 | ||
</Button> | ||
<FogEffect showFogEffect={showFogEffect} /> |
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.
p2: 상태 정의는 되어있는데 상태가 바뀌는 로직이 존재하지 않는 것 같아요. pr에 작성해주신 것처럼 아직 완벽하게 구현되지 못해서 그런걸까요?
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.
넵 맞습니다 구현하다가 로직 다 지워버려서 우선 state 만 남겨둔 상태입니다!
src/components/home/SearchModal.tsx
Outdated
|
||
const ScrollWrap = styled.div` | ||
overflow-y: auto; | ||
height: 800px; |
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.
p4: rem으로 수정해주세요.
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.
변경 커밋 날렸습니다
src/styles/theme.ts
Outdated
@@ -22,6 +22,7 @@ const colors = { | |||
grey30: '#C1C7CF', | |||
grey20: '#E0E3E5', | |||
grey10: '#EFF1F2', | |||
skygrey: '#EEF3F9', |
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.
p5: 대체 왜 스카이그레이만 쏙 빠졌던 것일까요... 추가해주셔서 감사합니다
src/pages/home/index.tsx
Outdated
}); | ||
|
||
const onModalToggle = (modal: 'search' | 'calender') => { | ||
console.log(isModalOpen); |
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.
p5: 나중에 home 페이지랑 합치고 나서 콘솔로그 지우면 좋을 것 같아요!
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.
수정 커밋 날렸습니당
src/components/home/CityGroup.tsx
Outdated
const CityButton = ({ name, onClick }: buttonProps) => { | ||
return <StyledButton onClick={onClick}>{name}</StyledButton>; | ||
}; |
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.
p5: 한 파일 내 이렇게 컴포넌트 만들어서 써도 되는군요..? 저는 props 필요한 컴포넌트 필요할 때마다 파일 분리해서 썼는데 간단한건 파일 내에 써도 되겠다는 생각이 드네요...
src/components/common/Button.tsx
Outdated
import styled, { css } from 'styled-components'; | ||
|
||
interface props { | ||
onClick: () => void; |
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.
p5: 제 코드에 Button 끌고와서 사용하다가 고치긴 했지만..!! disable 상태일때도 OnClick을 요구하더라구요.. 혹시 요건 별 문제 없으셨는지? 저는 그래서 OnClick에 물음표 추가하긴했는데 다른 방법이 있는건지 궁금합니다!
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.
안녕하세요 민정님 코리가 늦었죠!!!... 거의 고칠게 없어서 그냥 질문정도로만 달았습니다 ㅎㅎ 수고많으셨습니다~~~
모두 수정 완료 했습니당 |
# Conflicts: # src/assets/svg/index.ts # src/pages/home/home.tsx # yarn.lock
# Conflicts: # src/assets/svg/index.ts
🔥 Related Issues
✅ 작업 리스트
🔧 작업 내용
disabled 된 버튼 상태에서는 모달이 닫히지 않도록 구현하였습니다.
🧐 새로 알게된 점
🤔 궁금한 점
이 두가지 이슈를 중점으로 두고 몇시간 해보다가 시간이 좀 더 할애될 것 같아서
우선 내려두고 캘린더로 넘어가야할 것 같습니다.
우선 지금은 화면을 좀 더 길게하여 스크롤이 좀 더 길게 내려가도록 해서 눈속임을 해두었고,
후에 시간이 남는다면 돌아와서 진행해보도록 하겠습니다,,, ㅠ
📸 스크린샷 / GIF / Link
렌더링 될 때 모달이 무조건 열리게 하여 구현한 상태입니다. 머지할 때는 해제해두겠습니다!
2024-11-22.01.52.26.mov