-
Notifications
You must be signed in to change notification settings - Fork 0
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
[#134] 대회 페이지 시뮬레이션 영역 모달로 분리하기 #146
The head ref may contain hidden characters: "134-\uB300\uD68C-\uD398\uC774\uC9C0-\uC2DC\uBBAC\uB808\uC774\uC158-\uC601\uC5ED-\uBAA8\uB2EC\uB85C-\uBD84\uB9AC\uD558\uAE30"
Conversation
✅ Deploy Preview for algo-with-me ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
여기 변경사항은 useSimulations -> useSimulation으로 바뀌면서 생긴 것들이 다수고, 전체적으로 simulationInputs... 이렇게 시작하던 이름을 inputs 이렇게 바꾼 것들이 다에요
html:has(dialog[open]) { | ||
overflow: hidden; | ||
} |
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.
여기만 보시면 되는디, 다이얼로그(=모달)가 열려있으면 html에서 스크롤이 동작하지 않게 만드는 방법입니다.
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.
오오 감사합니다:D
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.
오 진짜 좋네요.
:has는 처음보네요. 저런 selector도 있군요.
scroll:none 이런식으로 스크롤을 없앨거라 생각드는데, overflow:hidden�해서 스크롤 자체를 없앤다 너무 좋네요.
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 function Modal({ onBackdropPressed, children, ...props }: Props) { | ||
const modal = useContext(ModalContext); | ||
const $dialog = useRef<HTMLDialogElement>(null); |
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.
여기에서 dialog 앞에 $을 붙이신 이유가 뭔가요?:
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.
DOM을 나타내는 경우엔 $를 붙여서 구분짓고 있습니다. 개인적인 코딩 습관? 이에요
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 ReactDOM.createPortal( | ||
<dialog | ||
ref={$dialog} | ||
className={cx(style, dialogStyle)} | ||
aria-modal="true" | ||
aria-labelledby="dialog-title" | ||
onClick={handleClickBackdrop} | ||
{...props} | ||
> |
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.
createPortal은 처음 보는데 이렇게도 쓸 수 있군요, 감사합니다!:D
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.
네 app이 되는 <div id="root">
밖에 컴포넌트를 추가하며 모달 구현에 많이 쓰입니다.
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.
LGTM:D
export function deepCopy<T>(value: T): T { | ||
return structuredClone(value); | ||
} |
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.
헉..?
Object.assgin
JSON.stringify, JSON.parse
가 아닌 structuredClone도 있네요?
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.
네 모든 객체 인스턴스를 커버하진 않지만 대부분의 객체, 배열에서 쓸 수 있습니다.
if (onBackdropPressed instanceof Function) { | ||
onBackdropPressed(); | ||
} |
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.
common 아래에 모달이라 onBackdropPressed가 있을수도 있고 없을 수도 있다는 고 있으면 실행해라는 의미로 해석됩니다.
typeof onBackdropPressed === 'function'로 안 하신 이유가 있을까요?
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.
별 의미는 없었습니다. typeof를 쓰는 것이 더 유리한가요?
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.
String으로 타입 비교 안 하고 'string'하는 것처럼 함수도 'function'으로 하는 게 일관성 있어 보여서 물어봤습니다. 찾아 봤는데, 아무런 문제 없어보이네요!
_backdrop: { | ||
background: 'rgba(00,00,00,0.5)', | ||
backdropFilter: 'blur(1rem)', |
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.
모달 만들때 꿀팁이네요.
backdrop-filter 속성 유용하네요
modal.close(); | ||
}; | ||
|
||
const handleSave = () => { | ||
onSave(deepCopy(inputs)); | ||
modal.close(); |
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.
useContext안에 모달을 닫고 여는 함수들의 네이밍(close,open)이 부실하다 생각했는데
modal로 받고 modal.close라고 하니 깔끔하고 좋네요.
simulation.run(code); | ||
}; | ||
|
||
const handleSimulationCancel = () => { | ||
cancelSimulation(); | ||
simulation.cancel(); |
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.
LGTM 😎
한 일
모달
리팩토링
스크린 샷
화면 구석에 '테스트 케이스 추가하기' 라는 버튼이 생겼습니다.
클릭 시 위와 같은 모달이 나타납니다.
테스트케이스를 입력하고 '저장'을 누르면 시뮬레이션이 저장됩니다.
저장을 하지 않고 닫기를 누르면 시뮬레이션이 적용되지 않습니다. 또한 다시 모달을 열어보면 저장하지 않은 입력이 사라진 것을 볼 수 있습니다.