[Mission5/김혜성] Project_Notion_VanillaJs 과제 #55
Conversation
src/events/documentTreeEvent.js
Outdated
|
|
||
| export const addDocumentButtonClickEvnet = async ({ event, target }) => { | ||
| const $input = document.createElement("input"); | ||
| $input.placeholder = "제목"; | ||
| $input.className = "documentInput"; | ||
| if (target === null) { | ||
| event.target.parentNode.insertBefore($input, event.target); | ||
| return; | ||
| } | ||
| target.appendChild($input); | ||
| }; |
There was a problem hiding this comment.
오.. 오.. 오타 발견!
그렇지만 이거를 말하려고 한 것은 아닙니다 ㅎ.ㅎ
add 버튼을 누르면 계속 제목 입력창이 생겨요! 그치만 이건 의도하신 걸 수도 있어서 좋지만.. 탈출구가 필요해 보입니다 ! add 버튼을 클릭하고, 제목을 입력하다가, 문서 생성을 취소할 수도 있으니까요..!
There was a problem hiding this comment.
아마 focus 랑 keyup 이랑 무언가가 이벤트가 겹친 듯 싶은데..!
add 버튼을 누르고 입력창이 나와서 무언가를 쓰면 add 버튼에 focus 가 향합니다!
그 상태에서 엔터키를 누르면 저렇게 제목 입력창들이 날아옵니다 !
There was a problem hiding this comment.
첫번째거는...저도 문제를 인지하고 있었는데 리팩토링 목록에 넣어두기만하고 아직 못고쳤습니다
탈출구도 만들어야겠네요!!
두번째는 제 환경에서는 일어나지 않는데 정확히 어떤 상황인가요?
There was a problem hiding this comment.
이 기능도 documentTree에 isInput state를 추가해서 해결했습니다!
| await request(`/documents/${id}`, { | ||
| method: "DELETE", | ||
| }).then((res) => { | ||
| removeDocumentFromStorage(res.id); | ||
| history.pushState(null, null, "/"); |
There was a problem hiding this comment.
const res = await request(`/documents/${id}`, { method: "DELETE" });
removeDocumentFromStorage(res.id);
history.pushState(null, null, "/");혹시 이런식으로 작성하지 않고, then 으로 이어 받은 이유가 있을까요? 궁금합니다 !
request 함수가 응답을 Promise 로 반환하지 않고, json 객체로 반환해주기 때문에 위와 같이 작성해도 무관하지 않을까..? 해서요! 그렇지만 확실하지 않기 때문에 혜성님 생각이 궁금합니다 : )
There was a problem hiding this comment.
세진님 말대로 서버에서 응답이 json객체로 오기때문에 이렇게 해도 좋겠네요!
저는 then이 프라미스를 다뤄주는 기능도 있지만 문맥적으로 then 내부에 서버에서 온 데이터로하는 작업들을 묶어두면 이해가 좀 더 쉽지 않을까??라는 생각이 들어서 이렇게 했어용
수정할거.md
Outdated
| 3. documnetId에대한 validation? | ||
| 4. 컴포넌트의 불변성? | ||
| 5. DocumentTree 렌더링 innerHTML로 바꿔서 렌더링 방식을 통일해주자 | ||
| 6. 무한 input |
There was a problem hiding this comment.
앗 제가 이거를 못보고 리뷰를 써버렸는데, 계획 중에 있으셨군요! 하핫
| background-color: #c4d7b2; | ||
| } | ||
|
|
||
| .documentLink { |
There was a problem hiding this comment.
여기서 documentLink, 즉 this.state.title 이 너무 길어지면 오른쪽의 documentButton 얘네가 잘리더라구요..?
text-overflow: ellipsis;
이 친구랑 너비 지정을 써서 특정 너비를 넘어가면 ... 으로 끝부분 생략이 되게 하는 것은 어떨까요 !
There was a problem hiding this comment.
와우와우와우와우 이런 좋은 속성이 있었군요!! 감사합니다 적용했어요!
src/router/route.js
Outdated
| switch (routeName) { | ||
| case "documents": | ||
| const savedDocument = await getDocument(documentId); | ||
| try { | ||
| const { title, content, tmpSaveDate } = | ||
| getDocumentFromStorage(documentId); | ||
| if (tmpSaveDate > savedDocument.updatedAt) { | ||
| if (confirm("임시저장된 데이터가 있습니다. 불러오시겠습니까?")) { | ||
| component.state = savedDocument.clone({ | ||
| title, | ||
| content, | ||
| updatedAt: tmpSaveDate, | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.log( | ||
| "임시저장된 데이터가 없습니다. 서버에서 데이터를 불러옵니다." | ||
| ); | ||
| } | ||
| component.state = savedDocument; | ||
| removeDocumentFromStorage(documentId); | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
제가 이 코드의 의도를 이해한 것이 맞는지 여쭤보려고 여기에 리뷰 남깁니다!
지금은 case 가 "documents" 하나지만, 더 많은 routeName 들을 사용하게 되었을 때를 대비해서 switch 문을 쓰신걸까요 ?? routeName 으로 나누어지는 기능들을 확장했을 때,
아래에 이어서 case "(routename)" 이렇게 쓰실 생각으로..!
2023-07-11.12.27.56.mov이렇게 추가되는 모습을 의도하신걸까요!? |
|
이렇게 CSS가 깨지고 있네요!? 2023-07-11.12.49.44.mov |
src/core/Component.js
Outdated
| #$target; | ||
| #state; | ||
| #events; | ||
| #allowedProperties = ["$target", "initialState", "events"]; |
There was a problem hiding this comment.
static #allowedProperties = ["$target", "initialState", "events"];이렇게 static 멤버 변수로 선언하면,
Component.#allowedProperties이렇게 메소드 내부에서 가져와서 사용할 수 있답니다!
이 두 가지가 어떤 차이가 있을까요?
| @@ -0,0 +1,77 @@ | |||
| export default class Component { | |||
| #$target; | |||
| if (typeof properties !== "object") | ||
| throw new Error( | ||
| `Component: properties는 object 타입이어야 합니다. 현재타입 : ${typeof properties}` | ||
| ); |
src/core/Component.js
Outdated
| constructor(properties) { | ||
| this.validate(properties); | ||
| this.#$target = properties.$target; | ||
| this.#state = properties.initialState; | ||
| this.#events = properties.events; | ||
| this.setEvent(this.#events); | ||
| this.render(); | ||
| } |
There was a problem hiding this comment.
지금 properties로 받고 있는데 이것 자체가 일종의 props가 아닐까? 해서 따로 선언하지 않았습니다...!
target,initialState,events 이것들이 전부 일종의 props니까 따로 props를 선언하지 않고 props에 해당하는 것들을 전부 propperties에 담아서 보내면 되지 않을까 해서요. props를 해체해서 보내준다고 말할수도 있겠네요...!
target과 initalState는 성격이 다르니까 따로 빼고 props를 선언하고 props안에 이벤트를 넣고 이외의 것들도 props에 넣어도 될것 같긴 한데... props안에 events를 넣으면 코드의 depth가 깊어지는것 같기도하고 뭐가 좋은지 잘 모르겠습니다ㅠㅠ
src/router/route.js
Outdated
| try { | ||
| const { title, content, tmpSaveDate } = | ||
| getDocumentFromStorage(documentId); | ||
| if (tmpSaveDate > savedDocument.updatedAt) { |
There was a problem hiding this comment.
이게 비교가 잘 될까요?.?
이게 아마 문자열끼리 비교하는게 될 것 같아서요!
There was a problem hiding this comment.
Date객체를 생성하여 비교하는 방식으로 바꿨습니다!
src/Component/Editor.component.js
Outdated
| <div class="editorContainer"> | ||
| <div class="title" ${ | ||
| this.state.id === -1 | ||
| ? `contentEditable="false"` | ||
| : `contentEditable="true"` | ||
| } name="title"> | ||
| ${this.state.title || "제목을 입력하세요"} | ||
| </div> | ||
| <div class="textarea" ${ | ||
| this.state.id === -1 | ||
| ? `contentEditable="false"` | ||
| : `contentEditable="true"` | ||
| } name="textarea"> | ||
| ${this.state.content || ""} | ||
| </div> |
There was a problem hiding this comment.
div 태그에는 name이 없답니다! 정식 스펙이 아니에요 ㅎㅎ
There was a problem hiding this comment.
찾아보니 정말 그렇군요... IE에서도 호환이 되지 않으니!
id나 class값으로 충분한데 굳이 name을 쓸필요는 없겠네요ㅎㅎ;
| export const textareaKeyupEvent = ({ title, content }) => { | ||
| clearTimeout(timeout); | ||
| timeout = setTimeout(() => { | ||
| saveDocumentToStorage({ title, content }); | ||
| }, 200); | ||
| }; | ||
|
|
||
| export const titleKeyupEvent = ({ title, content }) => { | ||
| clearTimeout(timeout); | ||
| timeout = setTimeout(() => { | ||
| saveDocumentToStorage({ title, content }); | ||
| }, 200); | ||
| }; |
| }).then((res) => { | ||
| if (res.content === null) { | ||
| res.content = ""; | ||
| } | ||
| return new Document(res); | ||
| }); |
There was a problem hiding this comment.
.then(Document.of);요런 느낌으로 메소드를 만들어주면 어떨까 싶기도 하네요 ㅋㅋ
| }).then((res) => { | ||
| return new DocumentTree({ documentTree: res, isInput: false }); | ||
| }); |
📌 과제 설명
바닐라 자바스크립트로 만든 노션 클로닝? 편집기
요구사항
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점
고민했던 점
api호출 시기에 관해
언제 api를 호출해야 사용성도 좋으면서 api호출을 적게 할 수 있을까 많은 고민을 했다. 가장 큰 고민은 에디터 내부의 내용을 저장하는 시기에 관한 것이었다. 제일 먼저 다른 링크로 라우팅 할 때 서버에 에디터 내용을 저장하면 api호출을 최대한 적게 할 수 있어 좋지 않을까 생각했지만 추후 다른 페이지들이 많이 생긴다면 분명히 사이드 이펙트가 생길 것 같았다.
결국 이런 저런 방법을 시도하다 에디터가 foucsout될 때 저장하는 api를 호출하는 것으로 구현했다.
추상화에 대해
저번 과제에 이어 컴포넌트와 도메인 데이터를 최대한 추상화하여 만들었다. 하지만 여전히 추상화가 가능한 지점들이 많이 보인다. 예를 들어 이벤트도 추상화 할 수 있을 것 같고 도메인 데이터도 결국 같은 모습을 띄기 때문에 그들의 인터페이스도 구현해 볼 수 있을것 같다.
데이터의 불변성에 대해
state에 새로운 데이터를 할당할 때 전부 새로운 도메인 모델을 new 생성자로 만들었다. 일부러 setter는 구현하지 않았다. 따라서 모든 state변경에 validation이 가능하고 데이터가 이상하게 바뀌는 사이드 이펙트를 줄일 수 있는 효과를 기대해 볼 수 있을 것 같다.
아쉬운 점
state를 먼저 수정하고 api응답 받기 전에 state를 기반으로 UI를 먼저 렌더링 해주면 더 좋은 사용자 경험을 제공할 수 있을지 않았을까??
사실 구현에 집중하느라 UI를 크게 신경 못썼다....하하.... 리팩토링 기간에 UI도 예쁘게 다듬어 보아야겠다.
처음부터 설계를 하고 코딩하려고 하니 감이 안잡혀서 일단 기능들을 모두 App.js에 때려넣고 구현한 뒤 하나하나 해체작업을 했다. 일단 빠르게 기능을 구현할 수 있다는 장점은 있었지만, 나중에는 전체적인 구조가 눈에 안들어오고 코드의 가독성도 심히 떨어졌다. 결국 처음부터 시간을 들여 설계를 하는 쪽이 더 효율적이고 시간도 더 단축 됐을것 같다.
아직 사용하기에는 허술한 점이 많다... 예를 들면 +버튼을 누르면 input이 무한정 생성된다...ㅋㅋㅋㅋ
궁금한 점
1.route함수에 component를 넘겨주는 방식이 마음에 들지 않는다. route는 현재의 component와 무관하게 입력된 url에 맞는 부분을 렌더링 해줘야 할 것으로 역할을 한계지어야 할 것 같은데 지금의 route는 너무 많은 일을 한다. 어떻게 하면 효율적인 route를 만들 수 있을까?
리팩토링 목록
재미로 보는 lighthouse점수
커밋기록을 보니 꽤나 꾸준히 했다. 뿌듯하다 하하