feat(apply): implement the function to preview the application form before final submission #649
Conversation
frontend/src/App.js
Outdated
| </ScrollToTop> | ||
| </main> | ||
| <Footer /> | ||
| <div id="modal-root" /> |
There was a problem hiding this comment.
c : portal을 이용해서 렌더링하는 경우, 가장 바깥쪽에 있는 계층에 위치하지 않나요? Docs 뿐만 아니라, react portal을 설명하는 대부분의 예제들에서 가장 바깥쪽 계층에 <div id="modal-root"></div>을 선언하네요.
There was a problem hiding this comment.
c: 저도 portal 영역을 여기 둔 이유가 있는지 궁금해요.
| overflow-y: auto; | ||
| font-size: 0.875rem; | ||
| line-height: 1.4rem; | ||
| text-align: justify; |
There was a problem hiding this comment.
a : justify(양쪽 정렬) 새롭게 알아가고, 디테일에 감탄합니다.
| margin-bottom: 0.5rem; | ||
| } | ||
|
|
||
| .application-item-description { |
There was a problem hiding this comment.
a : description과 answer-length가 딱 붙어 있어서, description과 answer의 구분이 명확하지 않는 것 같네요. 약간 띄우는 거는 어떨까요?
There was a problem hiding this comment.
description에 margin bottom 0.5rem 추가했습니다. 1b76f94
| } | ||
|
|
||
| .application-item-answer { | ||
| white-space: pre-line; |
| display: flex; | ||
| flex-direction: column; | ||
| gap: 2.5rem; | ||
| overflow-y: auto; |
There was a problem hiding this comment.
c : 스크롤 바와 컨텐츠가 겹치는데 살짝 띄우는 건 어떨까요?
There was a problem hiding this comment.
스크롤바 숨김처리 했습니다. 93272f5
| import classNames from "classnames"; | ||
| import styles from "./ModalWindow.module.css"; | ||
|
|
||
| interface ModalWindowProps |
There was a problem hiding this comment.
반영했습니다 😓 4f623bb
| className={classNames(styles.box, className)} | ||
| {...props} | ||
| > | ||
| <h2 id="dialogTitle" className={styles.title}> |
There was a problem hiding this comment.
r : 저는 title이 옵셔널 타입이어야 된다고 생각하고, title 유무에 따라서 title 영역이 렌더링되어야 한다고 생각합니다. 그 이유는 모달 컨텐츠 UI에 title이 없는 경우도 존재할 것 같습니다.
There was a problem hiding this comment.
모달창에서 title 제거했습니다 31b121d
| import { ModalContext } from "../hooks/useModalContext"; | ||
|
|
||
| export type ModalContextValue = { | ||
| Modal: (props: { children: NonNullable<React.ReactNode> }) => JSX.Element | null; |
There was a problem hiding this comment.
r : 해당 children 타입과 ModalPortal의 children prop의 타입이 동일해야 되지 않나요?
해당 children 타입
: NonNullable<React.ReactNode>
ModalPortal children 타입
: React.ReactNode
There was a problem hiding this comment.
..!! d567def
| closeModal: () => void; | ||
| }; | ||
|
|
||
| const ModalProvider = ({ children }: { children: React.ReactNode }) => { |
There was a problem hiding this comment.
r : ModalProvider도 컴포넌트이기에 type을 분리해주세요!
There was a problem hiding this comment.
분리했습니다. 0cf7ff6
| }; | ||
|
|
||
| return ( | ||
| <ModalContext.Provider value={{ Modal, openModal, closeModal }}> |
There was a problem hiding this comment.
a : 궁금한거 있는데요. Modal을 두 개 띄워야하는 상황이 온다면, 어떤 식으로 하실 생각이신지 궁금합니다. 🤔
There was a problem hiding this comment.
모달은 대화형 UI이기 때문에 한번에 하나씩만 등장해야 유저가 혼란스럽지 않고 태스크를 간단하게 유지할 수 있다고 생각합니다. 애플의 휴먼 인터페이스 가이드에서도 best practice로 모달 뷰 위에 다른 모달 뷰를 보여주는 것을 지양하도록 되어있습니다.
Avoid presenting a modal view on top of another modal view.
| Default.args = { | ||
| recruitmentItems: [ | ||
| { | ||
| recruitmentId: 1, | ||
| title: "프로그래밍 학습 과정과 현재 자신이 생각하는 역량은", | ||
| position: 1, | ||
| maximumLength: 1000, | ||
| description: | ||
| "우아한테크코스는 프로그래밍에 대한 기본 지식과 경험을 가진 교육생을 선발하기 때문에 프로그래밍 경험이 있는 상태에서 지원하게 됩니다. 프로그래밍 학습을 어떤 계기로 시작했으며, 어떻게 학습해왔는지, 이를 통해 현재 어느 정도의 역량을 보유한 상태인지를 구체적으로 작성해 주세요.", |
There was a problem hiding this comment.
a: 여기 데이터들 mock/dummy에 있어도 좋을 것 같아요.
There was a problem hiding this comment.
서버 데이터에 해당하는 recruitmentItems를 dummy.js로 이동했습니다. 2fd5ff2
answers와 다른 사용자 입력 데이터는 예시의 느낌으로 넣었기에 dummy.js로 이동시키지 않았습니다.
frontend/src/App.js
Outdated
| </ScrollToTop> | ||
| </main> | ||
| <Footer /> | ||
| <div id="modal-root" /> |
There was a problem hiding this comment.
c: 저도 portal 영역을 여기 둔 이유가 있는지 궁금해요.
There was a problem hiding this comment.
록바 & 소피아가 잘 리뷰해주셔서 🙂 코멘트 없는 부분에 대해서만 추가로 의견 남겨두었습니다.
추가 request changes 한가지는 디자인에 대한 부분이 있는데요.
- 모달의 '취소' 버튼이 Figma에서는 흰색 배경+테두리만 있는 버튼인데, 구현된 내용은 회색 배경이더라구요. 다른 영역들에서 이미 secondary 역할의 버튼을 테두리만 있는 스타일로 가져가고 있어, Figma와 스타일을 맞추는 게 어떨까요?
- 버튼의 너비도 Figma와 다르게 모달의 너비를 자동으로 채우게 구현해주셨는데, 요건 의도하신 부분이 맞을까요? 맞다면 Figma도 같이 업데이트 부탁드릴게요! (데일리에서 이야기해주셨던 것도 같은데 헷갈려서요)
- '제출한 뒤에는 수정할 수 없음을 확인했습니다' 체크박스는 가운데 정렬하는 것으로 논의했던 것 같은데 아닐까요?
- 모달에서의 '제출' -> '최종 제출'로 텍스트 변경하는 게 어떨까요? Figma에도 그렇게 되어 있기도 하고, 의미상으로도 제출 버튼을 클릭한 뒤 > 모달에서 또한번 제출하는 것이니 '최종으로' 제출한다는 의미가 될 것 같아서요
[Figma]
[구현]
frontend/src/App.css
Outdated
| @@ -5,6 +5,8 @@ | |||
| --pc-main-width: 800px; | |||
| --pc-main-width-narrow: 512px; | |||
|
|
|||
| --modal-window-z-index: 1020; | |||
| --modal-portal-z-index: 1010; | |||
There was a problem hiding this comment.
c: 아래의 --header-z-index, --tooltip-dimmed-z-index, --tooltip-z-index는 1씩 찔끔찔끔 올라오고 있었는데요! ㅋㅋㅋ 크게 문제될 건 아니지만 특별히 다른 이유가 없다면 이 파일 내에서 z-index 증가시키는 단위도 동일하게 가져가면 어떨까요?
There was a problem hiding this comment.
반영했습니다. 78dcd52
| } | ||
|
|
||
| .application-item-answer-length { | ||
| color: #999999; |
There was a problem hiding this comment.
r: 그레이스케일 색상으로 보이는데, color.css 의 색상 중 하나로 선택해서 활용하는 게 어떨까요? 위의 #666666도 마찬가지입니다!
There was a problem hiding this comment.
반영했습니다 6d3dfb8
| onClickConfirmButton: React.MouseEventHandler<HTMLButtonElement>; | ||
| }; | ||
|
|
||
| const ApplicationPreviewModalWindow = ({ |
There was a problem hiding this comment.
a: -Window까지 붙은 이유가 궁금합니당! ModalPortal이 있어서 쌍을 맞추기 위함일까요?
There was a problem hiding this comment.
ModalProvider에서 Modal이라는 컴포넌트를 내려주고있어 UI 적인 요소입을 나타내기 위해 window라는 이름을 붙였습니다.
| name="confirm-submit" | ||
| label="제출한 뒤에는 수정할 수 없음을 확인했습니다." | ||
| checked={isChecked} | ||
| onChange={(e: ChangeEvent) => { |
There was a problem hiding this comment.
c: 아래의 다른 handler들은 이미 따로 선언해두고 사용하고 있는데 요건 여기에서 직접 쓰신 이유가 있을까요?
There was a problem hiding this comment.
놓친 부분이네요..! 반영했습니다 e9019cc
| </Button> | ||
| <Button | ||
| type="submit" | ||
| variant="contained" |
There was a problem hiding this comment.
r: 여기에서 쓰이는 버튼들도 Button 내의 VARIANT 상수를 사용하도록 수정이 필요해보입니다 🙂
There was a problem hiding this comment.
반영했습니다. 9fd2363
| justify-content: center; | ||
| align-items: center; | ||
| background-color: rgba(0, 0, 0, 0.4); | ||
| animation: dissolve 0.3s forwards; |
| <div | ||
| className={styles.box} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Escape") closeModal(); |
There was a problem hiding this comment.
r: 현재 코드 베이스에서는 대부분 한 줄이더라도 괄호를 쓰고 줄바꿈을 하고 있는 것으로 알고 있는데요! 요것도 스타일 맞추는 게 어떨까요~?
There was a problem hiding this comment.
코드 스타일 수정했습니다 0f3fee4
frontend/src/App.css
Outdated
| @@ -5,6 +5,8 @@ | |||
| --pc-main-width: 800px; | |||
| --pc-main-width-narrow: 512px; | |||
|
|
|||
| --modal-window-z-index: 1020; | |||
| --modal-portal-z-index: 1010; | |||
There was a problem hiding this comment.
c: 아래는 쫌쫌따리로 1씩 올라오고 있었는데 추가해주신 부분에서는 갑자기 10씩 올라가게 되는데요! ㅋㅋ 같은 파일이니 이 안에서는 z-index 증가시키는 단위를 맞추면 어떨까요?
There was a problem hiding this comment.
반영했습니다. 78dcd52
|
|
||
| return null; | ||
| }; | ||
| export default ModalPortal; |
There was a problem hiding this comment.
c:
ModalPortal,ModalWindow는 충분히@common하위에 있어도 될 것 같은데 어떠신가요?
components/
- @common/
- Modal
- ModalPortal
- ModalWindow
- ApplicationPreviewModal/
- - 이건 추가로 의견이 궁금한 부분인데, 만약
@common하위로 옮겨서 공용 컴포넌트로 사용한다면, Provider도 해당 컴포넌트 하위에 두는 건 어떻게 생각하시나요? 디렉토리 구조상 'provider/' 가 따로 있다보니 컨벤션에 어긋나 보일 수는 있을 것 같은데... 역할로 보자면 최상위의provider/하위에 있는 것들은 실제로 앱 전체에 걸쳐서 필요할 수 있는 값인 반면 ModalContext는 Modal이라는 컴포넌트를 위해 필요한 것이면서 & Modal 컴포넌트를 쓰기 위해서는 반드시 같이 사용해야 하는 것이니 Modal쪽에서 제공해주도록 하는 것도 괜찮을 것 같아서요 🙂
There was a problem hiding this comment.
모달 구조 수정했습니다. f778095
- ModalPortal과 ModalWindow를
@common으로 이동했습니다. - ModalProvider를 hook 상단으로 이동시켰습니다.
- ModalWindow와 ModalPortal을 ModalProvider 내부에서 합쳐 Modal이라는 컴포넌트로 내려주도록 했습니다.
- ApplicationPreviewModal을 component 바로 하위로 이동시켰습니다.
add margin bottom 0.5rem to description Refs: #649 (comment)
Refs: #649 (comment) Co-authored-by: Kimsanglok <[email protected]>
- add dim color variable - change order of keyframes - remove border bottom of modal window in mobile media query - set max-height of modal window Refs: #649 (comment) #649 (comment) #649 (comment) #649 (comment)
- create modal root at initial rendering - set ref initial value as null Refs: #649 (comment) #649 (comment)
- remove scroll bar - change padding, size in mobile media query Refs: #649 (comment)
- move ModalPortal and ModalWindow to @common - move provider to hook - combine ModalWindow and ModalPortal in ModalProvider Refs: #649 (comment)
|
Refs: #649 (comment) Co-authored-by: Kimsanglok <[email protected]>
- add dim color variable - change order of keyframes - remove border bottom of modal window in mobile media query - set max-height of modal window Refs: #649 (comment) #649 (comment) #649 (comment) #649 (comment)
- create modal root at initial rendering - set ref initial value as null Refs: #649 (comment) #649 (comment)
- remove scroll bar - change padding, size in mobile media query Refs: #649 (comment)
- move ModalPortal and ModalWindow to @common - move provider to hook - combine ModalWindow and ModalPortal in ModalProvider Refs: #649 (comment)
- nest block - declare handler function Refs: #649 (comment)
greenblues1190
force-pushed
the
feature/application-form-preview
branch
from
a547e86 to
fc98c15
Compare
지원서 미리보기에서 답변 paragraph에 긴 문자가 있는 경우를 위해
|
woowahan-pjs
changed the title
Resolves #614
해결하려는 문제가 무엇인가요?
어떻게 해결했나요?
최종 제출 전에 지원서를 미리 볼 수 있는 기능을 기획하고 구현한다.
ModalWindow
ModalPortal을 통해div#modal-root에 리액트 포탈 생성ModalProvider로ModalPortal과openModal,closeModal함수 전달ModalWindow에서useModalContext로 함수를 전달받아 비어있는 모달창 구현ModalWindow를 이용하여ApplicationPreviewModalWindow와 같은 다양한 모달창을 구현할 수 있도록 했습니다.ApplicationPreviewModalWindow 동작 데모
ESC로 모달창 닫기
MDN에 따르면 dialog 요소는 항상 autofocus 속성을 가지는 컨트롤 요소(input, button)이 있어야 한다고 합니다. 모달창의 베이스가 되는 컴포넌트인
ModalWindow에는 재사용성을 위해 아무런 버튼도 배치하지 않았기 때문에 실제로 사용되는 모달을 구현할 경우에는 항상 모달 내부에autofocus속성을 가지는 요소를 배치해야 합니다. (이는 ModalPortal에서 ESC 키다운 이벤트를 받기 위해서도 필수적입니다.)ApplicationPreviewModalWindow에 포함된Checkbox의 예Storybook에서 MemoryRouter 버그를 수정한다.
preview.js의 전역 데코레이터에서MemoryRouter제거Route제거어떤 부분에 집중하여 리뷰해야 할까요?
RCA 룰
r: 꼭 반영해 주세요. 적극적으로 고려해 주세요. (Request changes)
c: 웬만하면 반영해 주세요. (Comment)
a: 반영해도 좋고 넘어가도 좋습니다. 그냥 사소한 의견입니다. (Approve)