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

fix: paste wysiwyg list #1230

Merged
merged 3 commits into from
Oct 19, 2020
Merged

fix: paste wysiwyg list #1230

merged 3 commits into from
Oct 19, 2020

Conversation

js87zz
Copy link
Contributor

@js87zz js87zz commented Oct 15, 2020

Please check if the PR fulfills these requirements

  • It's the right issue type on the title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has a description of the breaking change

Description

  • paste the wysiwyg list data considering current list depth

  • After fixing
    2020-10-15 17-48-51 2020-10-15 17_49_11


Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

* @returns {number} depth
* @private
*/
_getContinuousDepth(el) {
Copy link
Member

Choose a reason for hiding this comment

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

이건 스콰이어에서 자식 리스트를 li 하위가 아니라 nextSlibling으로 생성하는 이슈 때문에 _getListDepth와 분리해서 생성된 것이죠~?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앞에 전처리 과정에서 붙은 ul, li 태그들 때문에 추가된 depth를 현재 depth랑 비교하기 위해 추가하였습니다. 말이 조금 어렵네요..

Comment on lines 602 to 607
_getRemovedUnnecessayListWrapper(el, orgEl) {
while (el.querySelectorAll('ul,ol').length > orgEl.querySelectorAll('ul,ol').length) {
el = el.firstChild;
}
return el;
}
Copy link
Member

Choose a reason for hiding this comment

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

이 로직은 아래와 같은 경우를 처리할 때가 맞나요??

// el
<ul>
  <li></li> ---> 반환
  <ol>
    <li></li>
  </o>
</ul>
// orgEl
<ul>
  <li></li>
</ul>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

만약에 복사한 원본 리스트가 아래와 같은데

<ul>
  <li>
    <ul>
      <li></li>
    </ul>
  </li>
</ul>

기존에 전처리 로직을 타게 되면 아래처럼 depth를 더 추가해서 이상하게 변경하더라구요. 그래서 원본 데이터에 맞게 depth를 보정하는 내용입니다.

<ul>
  <li>
    <ul>
       <li>
         <ul>
           <li></li>
         </ul>
       </li>
    </ul>
  </li>
</ul>

@@ -420,7 +420,7 @@ describe('WwPasteContentHelper', () => {

expect(element.childNodes.length).toEqual(1);
expect(element.childNodes[0].tagName).toEqual('UL');
expect($(element.childNodes[0]).find('li > ul > li > ul > li').length).toEqual(2);
expect($(element.childNodes[0]).find('li > div').length).toEqual(2);
Copy link
Member

Choose a reason for hiding this comment

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

div로 바뀐 이유는 무엇인가요~?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

불필요한 depth를 제거해서입니다~

@seonim-ryu
Copy link
Member

테스트를 하다보니 이런 케이스가 발견되었는데요 ㅜㅜ 복사된 리스트의 시작점이 같은 뎁스의 리스트가 아니라 다른 경우에 첫 번째 리스트 아이템 이후의 리스트 아이템들은 모두 삭제가 되고 있는데 이런 경우도 고려해야될 것 같습니다. (프로즈미러도 고려는 안된 케이스 같아서 처리가 어렵다면 새 이슈로 등록해서 처리해도 될 것 같습니다~)
test

@seonim-ryu
Copy link
Member

[10/16] 리뷰 완료합니다! 역시나.. 간단하지 않은 이슈였군요 ㅜㅜ <ul><ul><ul>... 이렇게 n차 뎁스를 가진 것들도 고려를 했어야 했네요. 일단 위에 코멘트한 케이스를 제외하고는 잘 동작하고 있습니다. 👍

Copy link
Contributor

@lja1018 lja1018 left a comment

Choose a reason for hiding this comment

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

[10/16] 리뷰완료
고생하셨습니다!

apps/editor/src/js/wwPasteContentHelper.js Outdated Show resolved Hide resolved
@js87zz js87zz merged commit 90143c4 into master Oct 19, 2020
@js87zz js87zz deleted the fix/paste-list branch October 19, 2020 01:49
@seonim-ryu seonim-ryu added the Bug label Oct 21, 2020
@seonim-ryu seonim-ryu added this to the editor@2.5.0 milestone Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants