-
Notifications
You must be signed in to change notification settings - Fork 5
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
Refactor/#120 클라이언트 crdt 라이브러리 적용 #128
The head ref may contain hidden characters: "Refactor/#120_\uD074\uB77C\uC774\uC5B8\uD2B8_CRDT_\uB77C\uC774\uBE0C\uB7EC\uB9AC_\uC801\uC6A9"
Conversation
} else { | ||
// tab: 들여쓰기 증가 | ||
console.log("tab"); | ||
const maxIndent = 3; |
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.
maxIndent는 따로 constants에서 관리하는게 좋겠군요 !
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.
정하는거 찬성입니당!! 지금처럼 3개가 좋아보입니당!
} | ||
} else { | ||
// tab: 들여쓰기 증가 | ||
console.log("tab"); |
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.
console.log가 있습니닷!
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.
앗 수정하겠습니다!
/* eslint-disable jsx-a11y/no-static-element-interactions */ | ||
import { BlockCRDT } from "@noctaCrdt/Crdt"; | ||
import { Block as CRDTBlock, Char as CRDTChar } from "@noctaCrdt/Node"; | ||
import { BlockId } from "@noctaCrdt/NodeId"; |
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.
수고하셨습니다!
Block은 컴포넌트 이름으로 활용하고 계셔서 여기서 CRDTBlock으로 바꿔서 사용하셨군요!
DB스키마 상에서는
BlockCRDT -> Block 안에 텍스트를 출력할때 관리하는 CRDT고
EditorCRDT -> 텍스트 Block 단위를 링크드리스트로 관리하는 CRDT로 알고있습니다.
올바르게 잘 적용하신 것 같습니다.
CRDTBlock과 BlockCRDT.. 이 네이밍은 어떻게 해야 명시성이 있을지 고민을 해봐야겠네요.🤔
const handleClick = (e: React.MouseEvent) => { | ||
e.preventDefault(); | ||
onClick(node.id); | ||
}; |
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.
상당히 block관련 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.
아직 덜 봤습니다...! 오늘(일요일)까지 보겠습니닷!! 2번째 버그도 같이 살펴보겠습니당!
그리고, 사소한 부분 (컨벤션 집착..)에 코멘트를 많이 달았습니다,, 연규님이 생각하시기에 아니라고 생각되시는 부분은 편하게 말씀해주세요!!
그리고,, 코드 보면서 감탄했습니다,,, 정말,, 고생하셨습니다,,,, 👍👍👍 최곱니다,,, 🙇♂️
앗 그런데 git flow 방식이, Feature, Release, Hotfix 이렇게 나뉘어집니다!
그래서 지금 붙혀진 Refactor가 아니라 Feature가 맞을거에요! 다음에는 Feature로 해주시면 좋을 것 같습니당! 앞에 붙히는거는 git flow init으로 초기설정 가능합니다!
const [editorState, setEditorState] = useState<EditorStateProps>({ | ||
clock: editorCRDT.current.clock, | ||
linkedList: editorCRDT.current.LinkedList, | ||
currentBlock: null as BlockId | 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.
요 부분은 그냥 null로 해도 될 것 같은데, as를 붙히신 이유가 있을까요??
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.
props에서 타입이 blockId | null이라 맞춘 것 같은데 빼도 상관 없을 것 같습니다!
export const useMarkdownGrammer = (props: { | ||
editorCRDT: EditorCRDT; | ||
editorState: EditorStateProps; | ||
setEditorState: React.Dispatch< | ||
React.SetStateAction<{ | ||
clock: number; | ||
linkedList: BlockLinkedList; | ||
currentBlock: BlockId | null; | ||
}> | ||
>; | ||
}) => { | ||
const { editorCRDT, editorState, setEditorState } = 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.
요 부분은 따로 Interface로 분리하고, 처음부터 구조분해할당으로 가져오면 더 좋을 것 같아요!
export const useMarkdownGrammer = (props: { | |
editorCRDT: EditorCRDT; | |
editorState: EditorStateProps; | |
setEditorState: React.Dispatch< | |
React.SetStateAction<{ | |
clock: number; | |
linkedList: BlockLinkedList; | |
currentBlock: BlockId | null; | |
}> | |
>; | |
}) => { | |
const { editorCRDT, editorState, setEditorState } = props; | |
interface useMarkdownGrammerProps { | |
editorCRDT: EditorCRDT; | |
editorState: EditorStateProps; | |
setEditorState: React.Dispatch< | |
React.SetStateAction<{ | |
clock: number; | |
linkedList: BlockLinkedList; | |
currentBlock: BlockId | null; | |
}> | |
>; | |
} | |
export const useMarkdownGrammer = ({ | |
editorCRDT, | |
editorState, | |
setEditorState, | |
}: useMarkdownGrammerProps) => { |
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.
좋은 것 같습니다 반영하겠습니다!
|
||
const handleCompositionStart = useCallback(() => { | ||
setIsComposing(true); | ||
useEffect(() => { |
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.
useEffect은 제일 하단에 위치시키기로 했었습니당!
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.
확인했습니다!
} else { | ||
const prevBlock = | ||
currentIndex > 0 ? editorCRDT.LinkedList.findByIndex(currentIndex - 1) : null; | ||
if (prevBlock) { | ||
currentContent.split("").forEach((char) => { | ||
prevBlock.crdt.localInsert(prevBlock.crdt.read().length, char); | ||
}); | ||
prevBlock.crdt.currentCaret = prevBlock.crdt.read().length; | ||
editorCRDT.localDelete(currentIndex); | ||
updateEditorState(prevBlock.id); | ||
} |
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.
1번째 버그 (1)
- 블록에 텍스트가 있고, 첫번째 캐럿에서 백스페이스를 눌러 이전 블록과 텍스트를 합칠 때, 마지막 글자가 사라지는 버그
- ex) 123 + 45 => 1234
해당 부분을 먼저 이렇게 바꿔야할 것 같습니다!
} else { | |
const prevBlock = | |
currentIndex > 0 ? editorCRDT.LinkedList.findByIndex(currentIndex - 1) : null; | |
if (prevBlock) { | |
currentContent.split("").forEach((char) => { | |
prevBlock.crdt.localInsert(prevBlock.crdt.read().length, char); | |
}); | |
prevBlock.crdt.currentCaret = prevBlock.crdt.read().length; | |
editorCRDT.localDelete(currentIndex); | |
updateEditorState(prevBlock.id); | |
} | |
} else { | |
const prevBlock = | |
currentIndex > 0 ? editorCRDT.LinkedList.findByIndex(currentIndex - 1) : null; | |
if (prevBlock) { | |
const prevBlockEndCaret = prevBlock.crdt.read().length; // ✅ 여기서 먼저 마지막 위치 계산 (prevBlockEndCaret) | |
currentContent.split("").forEach((char) => { | |
prevBlock.crdt.localInsert(prevBlock.crdt.read().length, char); | |
}); | |
prevBlock.crdt.currentCaret = prevBlockEndCaret; // 🚨 여기서 prevBlock.crdt.read().length 로 계산해버리면, 이미 crdt로 insert 다 된 상태에서 마지막 caret위치를 가져오게 됩니다. | |
editorCRDT.localDelete(currentIndex); | |
updateEditorState(prevBlock.id); // 🚨 또한 여기서 state 값을 바꾸면서, Editor.tsx -> handleBlockInput -> useEffect에서 텍스트 삭제 로직이 돕니다. -> 그래서 삭제로직에서 마지막 caret 위치를 아예 삭제해버립니다. (2번째 버그 설명에서 더 자세하게 설명하겠습니다!) | |
e.preventDefault(); // ✅ 그래서 이거를 추가했습니다! 그러면 텍스트 삭제 로직이 돌지 않습니다. 만약 해당 코드를 추가하지 않으면 이젠 1번째 블록의 마지막 글자가 사라집니다 ex) 123 + 45 => 1245 | |
} | |
} |
2024-11-17.4.53.54.mov
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.
감사합니다!!! 반영하겠습니다!
block.crdt.localDelete(caretPosition); | ||
block.crdt.currentCaret = caretPosition; |
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.
1번째 버그 (2)
- 이 코드가 범인이었습니다.. 여기에 console.log("너가 범인이다!") 하면 출력이 됩니다..
- 이유는 아까 useMarkdwonGrammer.tsx -> updateEditorState(prevBlock.id); 에서 state값을 바꾸면서, 해당 코드가 돌게 됩니다. 여기서 텍스트 삭제 로직이 돌고, caret 위치에 따라 삭제됩니다.
코드가,, 한줄 밖에 없을 겁니다.. 핫핫.. |
width: "full", | ||
minHeight: "spacing.lg", | ||
margin: "spacing.sm 0", | ||
padding: "spacing.sm", |
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.
요거 그냥 "sm"으로 해도 동작됩니다!
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.
alignItems: "center", | ||
position: "relative", | ||
width: "full", | ||
minHeight: "spacing.lg", |
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.
spacing 이 간격 관련해서만 sm, md, lg 픽셀값을 정한거라, "spacing.lg"가 아니라 "lg"로 불러올 수 있도록 해놨습니다!
그런데 minHeight은 간격 관련된게 아니라, "lg"로 하면 적용이 안될겁니다.. 연규님이 하신 방식이 적용이 되는 이유는, 아예 토큰 값 자체를 불러와서 적용이 되는 겁니당! 그래서 이건 간격 관련이 아니라 아예 "16px"등으로 값을 쓰는게 더 좋아보여요!
import { BlockCRDT } from "@noctaCrdt/Crdt"; | ||
import { Block as CRDTBlock } from "@noctaCrdt/Node"; | ||
import { BlockId } from "@noctaCrdt/NodeId"; | ||
import React, { memo, useEffect, useRef } from "react"; |
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.
React 18버전 이후부터는 따로 React를 import 안해줘도 될거에요!
|
||
export const blockContainerStyle = cva({ | ||
base: { | ||
...baseBlockStyle, |
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.
요렇게 뺀 것도 깔끔해보이는데요! 그렇지만 이렇게 하면 panda css eslint가 오류를 잡지 못합니다ㅜㅜ
규칙에 따라 자동으로 고칠 수도 있어서, 조금 더 일관성있는 코드로 작성할 수 있습니다!
textStyle: "display-medium16", | ||
flex: "1 1 auto", // 변경: flex-grow: 1, flex-shrink: 1, flex-basis: auto | ||
minWidth: "0", // 추가: flex item의 최소 너비를 0으로 설정 | ||
outline: "none", | ||
borderRadius: "radii.xs", |
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.
요것도 그냥 "xs"로 사용할 수 있습니다!
import React from "react"; | ||
|
||
interface IconBlocknProps { | ||
type: string; | ||
index?: number; // ol일 때 순서를 위한 prop | ||
} | ||
|
||
export const IconBlock: React.FC<IconBlocknProps> = ({ type, index }) => { | ||
let content; | ||
|
||
if (type === "ul") { | ||
content = ( | ||
<div style={{ marginRight: "8px" }}> | ||
<span>•</span> | ||
</div> | ||
); | ||
} else if (type === "ol") { | ||
content = ( | ||
<div style={{ marginRight: "8px" }}> | ||
<span>{index}.</span> | ||
</div> | ||
); | ||
} else if (type === "checkbox") { | ||
content = ( | ||
<div style={{ marginRight: "8px" }}> | ||
<input type="checkbox" /> | ||
</div> | ||
); | ||
} else { | ||
content = null; | ||
} | ||
|
||
return content; | ||
}; |
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.
React.FC 타입이 안정성에 있어서 지양하는게 좋다고 하더라구요..! (자세한건 조사를 못했습니다..)
그리고 type의 경우에도 nocta의 ElementType 을 재사용하면 좋을 것 같은데, 어떻게 생각하시나요??
import React from "react"; | |
interface IconBlocknProps { | |
type: string; | |
index?: number; // ol일 때 순서를 위한 prop | |
} | |
export const IconBlock: React.FC<IconBlocknProps> = ({ type, index }) => { | |
let content; | |
if (type === "ul") { | |
content = ( | |
<div style={{ marginRight: "8px" }}> | |
<span>•</span> | |
</div> | |
); | |
} else if (type === "ol") { | |
content = ( | |
<div style={{ marginRight: "8px" }}> | |
<span>{index}.</span> | |
</div> | |
); | |
} else if (type === "checkbox") { | |
content = ( | |
<div style={{ marginRight: "8px" }}> | |
<input type="checkbox" /> | |
</div> | |
); | |
} else { | |
content = null; | |
} | |
return content; | |
}; | |
import { ElementType } from "@noctaCrdt/Interfaces"; | |
interface IconBlockProps { | |
type: ElementType; | |
index?: number; | |
} | |
export const IconBlock = ({ type, index = 1 }: IconBlockProps) => { | |
const getIcon = () => { | |
switch (type) { | |
case "ul": | |
return "•"; | |
case "ol": | |
return `${index}.`; | |
case "checkbox": | |
return <input type="checkbox" />; | |
default: | |
return null; | |
} | |
}; | |
const icon = getIcon(); | |
if (!icon) return null; | |
return ( | |
<div style={{ marginRight: "8px" }}> | |
<span>{icon}</span> | |
</div> | |
); | |
}; | |
그리고 해당 코드처럼 중복되는 코드를 조금 더 줄일 수 있을 것 같아요!
const baseBlockStyle = { | ||
display: "flex", |
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.
혹시 block파일은 feature/editor/component 부분에 넣는거 어떻게 생각하시나요??
이건 editor 피처에서만 관리되는 것 같아서요! 여기 src/components 쪽은 버튼, 모달, 레이아웃같은 공통된 부분만 넣는걸로 생각했습니다!
@@ -0,0 +1,254 @@ | |||
import { EditorCRDT, BlockCRDT } from "@noctaCrdt/Crdt"; |
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.
여기 hook도 feature/editor/hook부분에 따로 빼는 거 어떨까요?? src/hooks은 정말 여러곳에서 쓰이는 hook을 모아둔다고 생각했습니다! (아직까지 그럴만한 hook은 나오지 않았지만요...)
@@ -4,38 +4,47 @@ const MARKDOWN_PATTERNS: Record<string, MarkdownPattern> = { | |||
h1: { | |||
regex: /^#$/, | |||
length: 1, | |||
createElement: () => ({ type: "h1" }), |
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.
여기 hooke도 feature/editor 쪽에 넣는건 어떨까요??
(e: React.KeyboardEvent<HTMLDivElement>) => { | ||
const createNewBlock = (index: number): Block => { | ||
const { node: newBlock } = editorCRDT.localInsert(index, ""); | ||
// TODO: 블록 타입이 초기화가 안됨??? |
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.
혹시 이건 무슨 버그인지 설명해주실 수 있을까요?? 궁금합니다!
client/src/utils/markdownPatterns.ts
Outdated
}, | ||
h3: { | ||
regex: /^###$/, | ||
length: 3, | ||
createElement: () => ({ type: "h3" }), | ||
createElement: () => ({ type: "h3", length: 3 }), |
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.
##, ###이 제대로 동작을 안하더라구요! 코드 순서를 ### -> ## -> # 으로 하면 기대한대로 동작이 되더라구요!
const MARKDOWN_PATTERNS: Record<string, MarkdownPattern> = {
h3: {
regex: /^###$/,
length: 3,
createElement: () => ({ type: "h3", length: 3 }),
},
h2: {
regex: /^##$/,
length: 2,
createElement: () => ({ type: "h2", length: 2 }),
},
h1: {
regex: /^#$/,
length: 1,
createElement: () => ({ type: "h1", length: 1 }),
},
서윤님 피드백 정말 감사합니다! 컨벤션 뿐만이 아니라 버그 수정도 해주셔서 일부 문제되는 부분들 해결할 수 있었습니다! 피드백 주신 부분 수정하고 push 하겠습니다! |
연규님 화이팅입니다 ! |
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 (blockRef.current && isActive) { | ||
const selection = window.getSelection(); | ||
if (!selection) return; | ||
|
||
const range = document.createRange(); | ||
const content = | ||
blockRef.current.firstChild || blockRef.current.appendChild(document.createTextNode("")); | ||
const position = Math.min(textCRDT.current.currentCaret, content.textContent?.length || 0); | ||
range.setStart(content, position); | ||
range.collapse(true); | ||
selection.removeAllRanges(); | ||
selection.addRange(range); | ||
} |
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.
연규님 연규님!!! 2번째 버그 해결했습니다... 아무리 디버깅해봐도 current Caret은 제대로 들어가는데, 화면상에서만 캐럿위치가 반영이 안되더라구요. 그래서 그냥 requestAnimationFrame
로 프레임 맞춰줬더니 해결했습니다 🥺🥺🥺
if (blockRef.current && isActive) { | |
const selection = window.getSelection(); | |
if (!selection) return; | |
const range = document.createRange(); | |
const content = | |
blockRef.current.firstChild || blockRef.current.appendChild(document.createTextNode("")); | |
const position = Math.min(textCRDT.current.currentCaret, content.textContent?.length || 0); | |
range.setStart(content, position); | |
range.collapse(true); | |
selection.removeAllRanges(); | |
selection.addRange(range); | |
} | |
const setFocusAndCursor = () => { | |
if (blockRef.current && isActive) { | |
const selection = window.getSelection(); | |
if (!selection) return; | |
const range = document.createRange(); | |
const content = | |
blockRef.current.firstChild || | |
blockRef.current.appendChild(document.createTextNode("")); | |
const position = Math.min( | |
textCRDT.current.currentCaret, | |
content.textContent?.length || 0, | |
); | |
range.setStart(content, position); | |
range.collapse(true); | |
selection.removeAllRanges(); | |
selection.addRange(range); | |
} | |
}; | |
requestAnimationFrame(() => { // ✅ 추가 | |
setFocusAndCursor(); | |
}); |
2024-11-18.1.06.48.mov
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.
서윤님이 피드백을 어마어마하게 남겨주셨군요..ㄷㄷ 수고많으셨습니다!
📝 변경 사항
🔍 변경 사항 설명
🙏 질문 사항
해당 버그를 수정해보려고 했는데 아직 해결이 되지 않았습니다... 이외에도 테스트 해보시고 버그나는 부분 있으면 피드백 주시면 수정하겠습니다!
📷 스크린샷 (선택)
2024-11-16.7.29.03.mov
✅ 작성자 체크리스트