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: pasting error in table when using custom sanitizer (fix #980) #981

Merged
merged 8 commits into from
May 13, 2020

Conversation

seonim-ryu
Copy link
Member

@seonim-ryu seonim-ryu commented May 12, 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


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

@seonim-ryu seonim-ryu requested a review from js87zz May 12, 2020 23:07
* @param {boolean} needHtmlText pass true if need html text
* @returns {string|DocumentFragment} result
*/
function finalizeHtml(html, needHtmlText) {
Copy link
Member Author

Choose a reason for hiding this comment

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

해당 함수 로직은 새니타이저 모듈 외에 다른 곳에서도 범용적으로 사용될 수 있는 로직이라고 판단되어 domUtils 모듈로 이동함

@seonim-ryu seonim-ryu force-pushed the fix/table-paste-with-custom-sanitizer branch from 14c9163 to 686efbb Compare May 13, 2020 00:44
Copy link
Contributor

@js87zz js87zz left a comment

Choose a reason for hiding this comment

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

리뷰 완료입니다!

expect(pch.wwe.getSanitizer).toHaveBeenCalled();
});

it('sanitizes content of container', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 테스트가 있으니 첫번째 테스트는 없어져도 되지 않을까요? 검증하려는게 결국 디폴트 새니타이저를 탄 후 원하는 결과가 나오는가 라서 이 테스트로 첫번째 케이스까지 다 커버되는 것 같아요.

Copy link
Member Author

@seonim-ryu seonim-ryu May 13, 2020

Choose a reason for hiding this comment

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

커스텀 새니타이저도 스파이로 걸지말고 단순한 새니타이저 하나 만들어서 처리해도 될 것 같다는 생각이 드네요ㅎ 수정할게요~

expect(tph.wwe.getSanitizer).toHaveBeenCalled();
});

it('returns html string as sanitized dom (DocumentFragment)', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 테스트에서도 역시 첫번째 케이스가 자동으로 커버되서 첫번째 케이스는 없어져도 되지 않을까요?

@js87zz
Copy link
Contributor

js87zz commented May 13, 2020

@seonim-ryu
제가 새니타이저 작업하면서 이전에 추가한 TC들이 있는데 그 TC들은 선임님이 이번에 작성하신 걸로 다 커버될 것 같아 없어져도 될 것 같습니다. 이 부분도 제거해주실 수 있을까요?
아 중복되는 부분은 없어졌네요. 남아있는 부분은 다른 부분을 테스트하는거라 있어도 되겠네요. 무시하셔도 됩니다ㅎㅎ

@seonim-ryu seonim-ryu merged commit c9295c7 into master May 13, 2020
@seonim-ryu seonim-ryu deleted the fix/table-paste-with-custom-sanitizer branch May 13, 2020 10:06
js87zz pushed a commit that referenced this pull request Jun 17, 2021
* feat: store number of backticks of code to data attr (resolve #981)
* refactor: apply code review (ref: #982)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants