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

하이라이팅+드레그엔드롭 구현 #90

Merged
merged 12 commits into from
Dec 20, 2024

Conversation

rhehfl
Copy link
Collaborator

@rhehfl rhehfl commented Dec 20, 2024

🔗 관련 이슈

#87 #89

📝작업 내용

퀴즈 유형 중 조합형에 대해서 드래그엔드롭 기능을 추가했습니다. 원래도 있긴했지만 버그가 있어서 잘 작동하지 않았는데 이번에 정상화 시켰습니다.
추가로 코드를 보여주는 부분에 대해 highlightjs 라이브러리를 사용하여 하이라이팅 기능을 구현했습니다. 해당 라이브러리에서 다양한 테마를 제공해서 추후 디자이너님과 상의 후 최종 테마를 결정하려고 합니다.

버그픽스로 모달이 띄워졌을 때 이전에 버튼 등에 포커스가 남아있어서 엔터키로 이 액션을 트리거하면 버그가 생겼는데
Modal 내부의 useEffect를 사용하여 이전에 포커스된 태그에 대해서 포커스를 해재시키는 코드를 추가했습니다.

🔍 변경 사항

  • 변경 사항 1
  • 변경 사항 2
  • 변경 사항 3

💬리뷰 요구사항 (선택사항)

#89 에 해당하는 커밋이 하나 남아있습니다. 이는 상점 꾸미기 아이템을 착용했을 때 프로필이 변하는 부분을 @bluetree7878 @zzzRYT 님 앞에서 테스트로 구현해봤는데 생각보다 괜찮게 나와서 커밋으로 남겼습니다.

useDnDStore을 전역상태로 만들었습니다. 훅으로 만들면 props drilling처럼 많은 함수와 상태를 전달해야해서 코드가 좀 더러워질거같긴 한데 다른곳에서 만약에 드레그 엔 드롭 기능을 만든다하면 상태가 공유되기 때문에 사이드 이펙트가 생길수도 있을 것 같습니다.
서론이 길었는데 현재는 store형태이지만 hook으로 전환할지 말지 고민이 드는데 이에 관해서 같이 고민해주시고 의견 남겨주시면 감사할 것 같습니다!

@rhehfl rhehfl added the ✨ Feature 기능 개발 label Dec 20, 2024
@rhehfl rhehfl self-assigned this Dec 20, 2024
@rhehfl rhehfl linked an issue Dec 20, 2024 that may be closed by this pull request
1 task
Comment on lines 26 to 60
onDragEnter={() => {
setOutsideDropZone(false);
setDragOverItem({ value: '', index });
}}
/>
) : (
<S.TextBlockButton
draggable
onDragStart={e => {
e.currentTarget.classList.add('drag-start');
setDragStartItem({ value: text, index });
}}
onDragEnter={() => setDragOverItem({ value: text, index })}
onDragEnd={e => {
e.preventDefault();
if (isOutsideDropZone) {
spliceUserResponseAnswer(index);
} else {
drop((dragStartItem, dragOverItem) => {
if (dragOverItem.value === '') {
setUserResponseAtIndex(
dragStartItem?.value,
dragOverItem?.index
);
spliceUserResponseAnswer(dragStartItem?.index);
} else {
swapUserResponseAnswer(
dragStartItem.index,
dragOverItem.index
);
e.currentTarget.classList.remove('drag-start');
}
});
}
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 부분은 따로 함수로 나누지 않은 이유가 있을까요?
그런게 아니라면 함수로 나눠서 관리하는게 좋아 보입니다☺️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image
표시해놓은 부분이 TextBlock컴포넌트입니다 위 부분 자체가 로직이 많아 따로 컴포넌트로 분리했는데요
제 생각이지만 이미 spliceUserResponseAnswer drop등과같은 함수로 로직 자체는 분리되어 있고 값에 따라 어떠한 함수를 호출하는지 자체는 이벤트 부분에서 결정하는것이 좀 더 읽기 쉬울 것 같았습니다!
이조차 함수로 분리해버린다면 너무 많은 추상화가 이루어 질 것 같았습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

그런가요🧐 제가 이 코드를 보고 든 감상은, evnet handler부분에 너무 복잡해 보인다 였습니다.

파일 자체를 쪼개서 추상화를 시키는게 아닌, 같은 파일 내부에서 함수의 이름만 쪼개서 관리하는건 어떨까 하는 생각이었습니다.

  const handleDragStart = (e: React.DragEvent<HTMLButtonElement>) => {
    e.currentTarget.classList.add('drag-start');
    setDragStartItem({ value: text, index });
  };

  const handleDragEnter = () => {
    setDragOverItem({ value: text, index });
  };

  const handleDragEnd = (e: React.DragEvent<HTMLButtonElement>) => {
    e.preventDefault();
    if (isOutsideDropZone) {
      spliceUserResponseAnswer(index);
    } else {
      drop((dragStartItem, dragOverItem) => {
        if (dragOverItem.value === '') {
          setUserResponseAtIndex(dragStartItem?.value, dragOverItem?.index);
          spliceUserResponseAnswer(dragStartItem?.index);
        } else {
          swapUserResponseAnswer(dragStartItem.index, dragOverItem.index);
          e.currentTarget.classList.remove('drag-start');
        }
      });
    }
  };

  const handleEmptyDivDragEnter = () => {
    setOutsideDropZone(false);
    setDragOverItem({ value: '', index });
  };

  const handleClick = () => {
    spliceUserResponseAnswer(index);
  };

... 

 return (
    <>
      {!text ? (
        <S.EmptyDiv
          draggable
          onDragEnter={handleEmptyDivDragEnter}
        />
      ) : (
        <S.TextBlockButton
          draggable
          onDragStart={handleDragStart}
          onDragEnter={handleDragEnter}
          onDragEnd={handleDragEnd}
          onClick={handleClick}
        >
          {text}
        </S.TextBlockButton>
      )}
    </>
  );

하나의 버튼에 많은 이벤트가 들어가 있는 구조다 보니, 이런 방식이 좀 더 어울리지 않나 하는 생각에 코멘트를 달아봤습니다🙃

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 이해했습니다 그런 의미에서는 @zzzRYT 님 말씀이 맞는 것 같습니다 태그 안에 이벤트 부분에 코드가 있으면 읽기 힘들다는 말에 동의합니다👍
이 부분에 대해서는 위쪽에 함수로 분리해놓는것이 좋을 것 같네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

d8a1f9d 커밋에서 수정했습니다👍

right: 17px;
`;

export const ProfileBox = styled.span<{ $isIcon: boolean }>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

isIcon 생각보다 괜찮아 보입니다...? 이미 컴포넌트명에 프로필 이미지라는걸 알려줘서 이해하는데 큰 어려움은 없는 것 같습니다🥸

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 그런가요? 이 부분에 대해서는 추후에 수정하려고 따로 언급은 안 했었는데 🙄
의견 감사합니다!

@bluetree7878
Copy link
Collaborator

현재는 store형태이지만 hook으로 전환할지 말지 고민이 드는데 이에 관해서 같이 고민해주시고 의견 남겨주시면 감사할 것 같습니다!

제 생각엔 현재의 store 형태를 유지하는 게 좋을 것 같습니다. hook으로 따로 분리해도 되겠지만 현재 사용하는 곳 외에 쓸만한 곳이 생각이 잘 안 나네요.

아니면 혹시 hook으로 분리하게 된다면 어떤 식으로 분리를 시켜서 전환을 할지 설명해주실 수 있을까요?

@rhehfl
Copy link
Collaborator Author

rhehfl commented Dec 20, 2024

@bluetree7878 만약 훅으로 분리하게 되었을 때의 차이점은 드레그 엔 드롭에서 사용되는 이벤트 함수와 상태를 전부 하위 컴포넌트에게 props로 전달시키게 할 것 같습니다. 그렇게 하면 각 DnD별로 독립적인 상태로 운용이 가능하게 되지만 위쪽에 설명한대로 props drilling 현상이 발생할 것 같습니다. 아니면 ContextApi로 필요한 부분에서만 사용하는 방법도 있을 것 같습니다

Comment on lines +25 to +55
* const highlightedCode = useCodeHighlight(code, [code], 'javascript');
*
* return (
* <pre>
* <code>
* {parse(dompurify.sanitize(addLineNumberCode), options)}
* </code>
* </pre>
* );
*/
const useCodeHighlight = (
code: string,
deps?: DependencyList,
language: string = 'javascript'
) => {
const [highlightCode, setHighlightCode] = useState<string>('');

useLayoutEffect(() => {
try {
hljs.configure({ ignoreUnescapedHTML: true });
const highlightedCode = hljs.highlight(code, { language }).value;
setHighlightCode(highlightedCode);
} catch (error) {
setHighlightCode(code);
}
}, deps ?? [code, language]);

return highlightCode;
};

export default useCodeHighlight;
Copy link
Collaborator

Choose a reason for hiding this comment

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

const highlightedCode = useCodeHighlight(code, [code], 'C');

코드 하이라이팅 할 언어의 기본값을 javascript로 하고 다른 언어로 하고 싶을 때 이런 식으로 바꿀 수 있다는 게 참 맘에 드네요...👍👍😮

@rhehfl rhehfl merged commit 71f875a into develop Dec 20, 2024
@rhehfl rhehfl deleted the modify/#87/quiz_drag_and_drop branch December 20, 2024 08:28
@rhehfl
Copy link
Collaborator Author

rhehfl commented Dec 20, 2024

이번엔 조금 빠르게 머지시키겠습니다 ㅎㅎ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 기능 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quiz drag and drop
3 participants