-
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] feat: 새로고침 및 창을 닫는 동작에 대해 경고 alert 띄우기 #851
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.
navigateConfirm 관련한 부분 확인 해주세요
@@ -14,7 +14,7 @@ const useCardFormModal = () => { | |||
closeModal, | |||
isOpen, | |||
isOpenModalDisablingBlocker: | |||
isOpen(CARD_FORM_MODAL_KEY.navigateConfirm) || isOpen(CARD_FORM_MODAL_KEY.submitConfirm), |
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.
breadCrumb를 사용한 react router 이동 시, confirm modal을 띄워주고 해당 모달에서 확인등으로 경로 이동하려면 navigateConfirm도 조건에 포함되야합니다
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.
[아까 바다와 나눴던 이야기 정리!]
isOpenModalDisablingBlocker
에 대한 주석이 작성 확인/내용 확인 모달이 열려 있는가? 로 되어 있어서 두 조건을 체크하는 조건식으로 변경했었어요.
그러다 이 변수가 react-router-dom의 useBlocker
훅을 사용해 라우팅을 막는 상태인가?를 체크하는 의미로 살짝 변경(?)돼서 그에 맞춰 변수명을 isUnblocked
로 바꿨습니다.
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.
navigateConfirm 관련한 부분 확인 해주세요
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.
고생했어요! 페이지의 동작을 확인해 보았는데, 리뷰 작성 확인 모달이 뜬 상태에서는 새로고침, 뒤로가기 모두 작동하네요. 모달을 한 번에 하나만 띄우게 하는 정책을 그대로 유지할지, 혹은 우선순위를 두어 모달을 여러 개 띄우는 것도 가능하게 할지 의논하면 좋을 것 같습니다 :)
/** | ||
* @param isOpenModalDisablingBlocker : 이동 확인 모달과 리뷰 제출 확인 모달 모달들이 열려있는 지 여부 (모달의 이동/제출 버튼 클릭 시 페이지 이동해야하기 때문에 필요) | ||
* @param isUnblocked : useBlocker로 라우팅을 막지 않는(다른 페이지로 이동 가능한) 상태인지를 의미함 |
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.
라우팅 가능한 상태인지를 좀 더 직관적으로 알 수 있도록 isNavigationEnabled
와 같은 이름은 어떤가요?
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.
아까 바다와 얘기 나눴을 때 변수명에 Navigation~만 사용하면 useBlocker
와 연관된 변수라는 느낌이 덜해서 block을 담아서 네이밍했던 것 같아요. 그런데 지금 보니 nav가 더 나을 것 같기도 하고... @바다
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.
절충안으로 isNavigationUnblocked
로 변경했습니다!
보기에 다소 이상할지라도, 다른 모달이 이미 열려 있던 상황에서 사용자가 실수로 뒤로가기를 눌렀을 때 페이지가 전환되면서 데이터가 싹 날아가는 현상을 막는 게 맞다는 생각이 들어 모달이 열려있어도 페이지 이동 시 확인 모달/새로고침 및 창 닫기 시 alert이 뜨는 방식으로 구현했습니다. 이때 제출 확인 모달에서 제출을 누르면 작성 완료 페이지로 넘어가는 url 변경 작업이 있어 제출을 선택하면 페이지 이동 확인 모달이 뜨는 문제가 있었는데, 이동 확인 모달이 등장하면 제출 확인 모달을 닫아버리는 구현도 했었는데, 제출 확인 모달이 떠 있던 상황에서 새 모달이 떴다고 기존 모달을 강제로 닫아버리는 동작이 부자연스러운 것 같아 그냥 모달 중첩이 가능한 채로 놔뒀습니다. |
지금 작성 페이지에 모달이 5개 있는데, 각자 텍스트 크기에 따라 반응형이 적용되어 있는 상황이라 모든 케이스에 대해 모달 크기를 같게 하는 작업이 꽤 힘들 것 같아요. 그리고 모달이 하나만 떠 있는 줄 알았는데 하나를 닫았을 때 텍스트만 다른 또다른 모달이 여전히 있는 걸 보는 것도 조금 부자연스러운 경험인 것 같다는 생각이 듭니다. |
ux를 고려한다면, 세션 스토리지에 저장된 내용을 복원할 것 인지 여부도 사용자에게 묻는게 맞을 것 같네요 |
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.
6차 데모데이안에 세션 스토리지에 답변을 저장하고 복원하는 모달을 띄우는 작업이 있어야할 것 같아서, 리뷰 작성 페이지 경로 이동 시 답변 삭제하는 기능이 우선이라 일단 approve하고 답변 작성 복원 모달 기능 구현을 기다릴게요
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.
세션 스토리지는 별도의 이슈에서 처리하는 거로 들어서 우선 이 PR은 Approve하도록 하겠습니다! 고생했어요
* fix: isOpenModalDisablingBlocker의 조건에서 navigateConfirm을 검사하던 문제 수정 * feat: useNavigateBlocker에 새로고침 및 페이지 삭제 전 alert를 띄우는 기능 추가 * refactor: useBlocker를 이용해 라우팅을 막아야 하는지를 판단하는 변수 이름을 isUnblocked로 수정 및 그에 맞게 조건문 변경 * refactor: useBlocker를 이용해 라우팅이 막혀 있는 상태인지를 나타내는 변수의 이름을 isNavigationUnblocked로 수정 * refactor: 제출 확인 모달이 띄워진 상황에서도 뒤로가기를 누르면 이동 확인 모달을 띄울 수 있도록 수정
🚀 어떤 기능을 구현했나요 ?
isOpenModalDisablingBlocker
의 값을 정할 때, 작성 페이지에서 사용하는 모달인 recheck 혹은 submitConfirm이 열려 있는지를 확인해야 하는데 페이지 이동을 확인하는 navigateConfirm의 상태를 체크하고 있던 버그를 수정했습니다. (만약 의도한 동작이었다면 알려주세요!)🔥 어떻게 해결했나요 ?
beforeunload 이벤트
useBlocker
와 달리 저 두 동작은 브라우저가 전적으로 책임지는 로직이라beforeunload
를 사용하지 않고서는 접근할 수가 없었습니다.대안을 찾아봤지만 아쉽게도
beforeunload
를 대체할 수 있는 방안을 찾지 못했습니다.대안으로
pagehide
이벤트가 많이 언급되는데, 이 이벤트는 클래스의 소멸자마냥 페이지가 언로드될 때 실행되는 이벤트라 UI 상호작용이 불가능합니다.그래서 아예 매커니즘을 바꿔 경고 모달을 띄우는 대신 세션 스토리지 혹은 서버에 중간 데이터를 저장하는 방식을 권장하고, 이 동작을
pagehide
로 처리하는 예제가 많았습니다.호환성 문제를 떠나 UX 측면에서도 경고 모달을 띄워주는 것보다 데이터를 로컬에라도 저장한 뒤 다시 돌아왔을 때 복원해주는 방식을 권장하는 것 같습니다.
개인적으로 답변을 로컬이나 세션 스토리지에 저장하는 게 낫다고 생각합니다만 논의된 바가 없고 시급한 문제는 아니라고 생각해 alert만 띄우는 코드로 PR 올립니다!
📝 어떤 부분에 집중해서 리뷰해야 할까요?
기존 구현에 맞춰
isOpenModalDisablingBlocker
를 이용했습니다. 즉 한 페이지에 2개 이상의 모달을 띄우지 않습니다. (현재 통일성을 위해 alert로 띄우는 경우도 막아놨습니다)그런데 이렇게 되면 작성 확인 모달이나 제출 확인 모달이 떠 있는 상황에서 새로고침 등을 하는 경우에는 이동 확인 모달을 띄울 수 없습니다. 리뷰를 다 작성한 상황에서 사용자가 실수로 새로고침을 누르면(이럴 확률이 적긴 하겠지만요)경고 없이 내용이 다 날아갑니다.
그래서
isOpenModalDisablingBlocker
제한을 해제하는 것도 고려했는데, 뒤로 가기 상황에서 문제가 됩니다.ㄴ 이런 느낌입니다. 근데 캡쳐본으로 다시 보니 거슬리는 것 같기도...🤣
ㄴ 모달 떠 있을 때 && 뒤로가기 이벤트일 때 새로고침, 창 닫기 동작 때처럼 alert 띄우기가 가능은 했습니다. 다만 저 동작들이 일어났을 때 띄워주는 alert(확인 누르면 새고고침, 취소 누르면 아무 일도 하지 않음)와 똑같이 동작하게 만들 수 있는지는 아직 모르겠습니다. 필요하다면 시도해볼게요
isOpenModalDisablingBlocker
제한이 꼭 필요하지 않음📚 참고 자료, 할 말
결론