Skip to content
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] 대시보드 뷰 테스크 버튼 더블클릭 시 모달 노출 막기 #162

Merged
merged 6 commits into from
Jul 17, 2024

Conversation

jeeminyi
Copy link
Member

작업 내용 🧑‍💻

  • 대시보드 뷰에서 task btn 클릭해도 모달이 노출되지 않습니다!

알게된 점 🚀

기록하며 개발하기!

  • 지원이가 pr에서 언급했었던 stopPropagation을 활용해서 모달 동작을 막아주었습니다! 모달을 동작하게 하는 함수 자체를 상위 컴포넌트에서 빼서 prop으로 내려주어야 하나 고민했었는데, 해당 이벤트를 활용해서 모달을 열어주는 함수에 onDoubleClick일 경우, 모달을 리턴하지 않는 로직으로 구현할 수 있었습니다>! 상위 컴포넌트에 onDoubleClick={(e) => e.stopPropagation} 을 인자로 내려주었어요! 사실 과정에서 gpt에게 도움을 받긴 했지만,,,,,덕분에 이해해버려서 몇 줄 안되는 코드지만 많이 배웠습니다.

리뷰 요구사항 💬

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

  • task btn 파일 내부의 모달을 여는 함수에 아래 코드를 추가해주었는데 혹시 다른 부분에서 활용할 때 사이드 이펙트 생긴다면 말씀해주세요!!
		if (onDoubleClick) {
			onDoubleClick(e);
			return;
		}
		const rect = e.currentTarget.getBoundingClientRect();
		const calculatedTop = rect.top;
		const adjustedTop = Math.min(calculatedTop, MODAL.SCREEN_HEIGHT - MODAL.TASK_MODAL_HEIGHT);
		setTop(adjustedTop);
		setLeft(rect.right + 6);
		setModalOpen((prev) => !prev);
	};

관련 이슈

close #161

스크린샷

2024-07-17.4.21.47.mov

Copy link
Member

@wrryu09 wrryu09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다!!!

@@ -13,6 +13,7 @@ import { TaskType } from '@/types/tasks/taskType';

interface BtnTaskProps extends TaskType {
btnType: 'staging' | 'target' | 'delayed';
onDoubleClick?: (e: React.MouseEvent) => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e를 직접 props에서 이벤트 핸들러를 통해 제어하기보다는 더블클릭을 막아야 하는 여부만 전달하는 것은 어떨까요? preventDoubleClick 같은 이름으로 boolean 타입 가지게 추상화하여 보다 명확하게 처리할 수 있을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 훨씬 직관적이겠네요! 반영하겠습니닷 감사합니다 ㅎㅎ

Comment on lines 45 to 48
if (onDoubleClick) {
onDoubleClick(e);
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (onDoubleClick) {
onDoubleClick(e);
return;
}
if (preventDoubleClick) {
e.preventDefault();
return;
}

위와 같이 수정하면 이 부분을 이렇게 고쳐볼 수 있을 것 같아요!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

또 event 중단 시 사용할 수 있는 방식이 여러가지 있는데, stopPropagation() 은 현재 이벤트가 상위로 전파되지 않도록 하는 것이고 event.preventDefault() 가 현재 이벤트의 기본 동작을 중단하는 방식이라 preventDefault() 를 사용하는 것은 어떨지 의견이 궁금해요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오,, 좋은 의견 감사합니다! 찾아보니 제안주신 preventDefault()는 고유의 동작을 막는 방식이라 요 로직에는 해당 방식이 더 적합할 것 같네요!! 반영하도록 하겠습니다.

@jeeminyi jeeminyi force-pushed the feat/#161/handle_modal branch from 4ad68bf to 1a92064 Compare July 17, 2024 08:33
Copy link
Member

@seong-hui seong-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

더블 클릭을 막기위해서 e.preventDefault를 사용해서 고유의 동작을 막으셨군요! boolean값으로 이를 판단하는 것도 직관적이고 좋은 것 같습니다 수고하셨습니당!! 👍

@@ -13,6 +13,8 @@ import { TaskType } from '@/types/tasks/taskType';

interface BtnTaskProps extends TaskType {
btnType: 'staging' | 'target' | 'delayed';
// onDoubleClick?: (e: React.MouseEvent) => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 함수가 기존에 더블클릭을 막고자 사용했다가 preventDoubleClick의 boolean값으로 처리하게 변경한 것으로 보여서 혹시 해당 코드가 필요하지 않다면 주석을 지워도 좋을 것 같습니다! 해당 코드의 주석을 남겨두신 이유가 있으실까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

으앙 확인하지 못했어요ㅣ,, 지울게욥 ㅜ! 꼼꼼하게 봐주셔서 감사합니닷

@jeeminyi jeeminyi merged commit 8b82068 into develop Jul 17, 2024
2 checks passed
@wrryu09 wrryu09 deleted the feat/#161/handle_modal branch July 18, 2024 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEAT] 대시보드 뷰 테스크 버튼 더블클릭 시 모달 노출 막기
3 participants