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

feat: apply toastmark parser to merged table plugin #1011

Merged
merged 19 commits into from
Jun 9, 2020

Conversation

js87zz
Copy link
Contributor

@js87zz js87zz commented Jun 1, 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

  • apply the toastmark parser to merged table plugin.
  • Advantages
    • Uniformity of parsing the markdown table.
    • Parse the markdown table independently from DOM structure.
    • Could Highlight the merged table in preview.
    • Simplify the parsing markdown code.

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

);
}

return {};
Copy link
Member

Choose a reason for hiding this comment

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

plugins가 없을때를 먼저 체크하고 빈 객체를 반환해주는 조건을 먼저 처리해주면 if문도 하나 줄고 가독성에도 좋은 것 같아요ㅎ

if (!plugins) {
  return {};
}

return plugins.reduce( /* */ );

@@ -0,0 +1,81 @@
function extractPropertiesForMerge(value, type, oppossitType) {
Copy link
Member

Choose a reason for hiding this comment

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

oppossitType -> oppositeType 이용ㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

기존 함수의 폐해군요..ㅋㅋ

@@ -0,0 +1,81 @@
function extractPropertiesForMerge(value, type, oppossitType) {
Copy link
Member

Choose a reason for hiding this comment

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

근데 oppositeType에는 어떤 값이 들어오나요??
퍼블릭 함수는 아니지만 type이나 oppositeType 값에 대한 주석이 있으면 좋겠습니다.
(이럴때 타입이 없으니 불편하군요 ㅜㅜ)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

기존 함수 주석 살려 놓을게요~!

/**
 * Extract properties for merge.
 * @param {string} value - value
 * @param {string} type - merge type like colspan, rowspan
 * @param {string} oppositeType - opposite merge type
 *                                if merge type is colspan, opposite merge type is rowspan
 * @returns {Array.<number|string>} - returns merge count and value
 * @private
 */


function parserTableCellWithRowspanMap(node, parent, rowspan) {
const prevRow = parent.prev;
const columnLen = parent.parent.parent.columns.length;
Copy link
Member

Choose a reason for hiding this comment

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

parent 값이 이 값들과 맞나요? parent.parent 값 유무를 체크할 필요는 없을지 궁금해서 질문해봅니다ㅎ

parent = `tableRow`
parent.parent = `tableBody`
parent.parent.parent = `table`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아래 같은 경우도 있고 나머지는 말씀하신대로 입니다! 파싱로직 내에서 parent.parentnull 인 경우가 없어 별도 체크는 하지 않아도 될 것 같습니다.

parent.parent = `tableHead`

* @fileoverview Test merged table parser
* @author NHN FE Development Lab <dl_javascript@nhn.com>
*/
import { source } from 'common-tags';
Copy link
Member

Choose a reason for hiding this comment

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

에디터 적용하는 피처에 있었는데ㅋㅋㅋ 역시 빠르시군요 👍

];

examples.forEach(({ no, content, result }) => {
it(`example1${no}`, () => {
Copy link
Member

Choose a reason for hiding this comment

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

example1${no} -> example${no} 요렇게 되어야하지 않을까요ㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉..감사합니다

@seonim-ryu
Copy link
Member

이렇게 데이터가 들어간 경우에 테이블이 깨지는데 확인 부탁드릴게요~

| @cols=2:merged |
| --- | --- |
| table | table |
  • Before
    before

  • After
    demo

Comment on lines 169 to 173
if (entering) {
inEmph = true;
} else {
inEmph = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

테스트 코드라 크게 문제되는건 아니지만.. 이렇게 처리할수도 있겠네요ㅎ

inEmph = entering;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commonmark 예제를 그대로 가져오다보니 그렇네요. 수정하겠습니다ㅎㅎ

Copy link
Member

@seonim-ryu seonim-ryu left a comment

Choose a reason for hiding this comment

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

[6/3] 데모를 띄워보느라 리뷰가 길어졌네요 😭 코멘트 달아둔 것 중에 파싱이 깨지는 경우가 있어서 요 부분 확인 부탁드릴게요! 짧은 시간 내 개발하시느라 고생 많으셨어요ㅎ

@js87zz
Copy link
Contributor Author

js87zz commented Jun 3, 2020

@seonim-ryu

이렇게 데이터가 들어간 경우에 테이블이 깨지는데 확인 부탁드릴게요~

| @cols=2:merged |
| --- | --- |
| table | table |
  • Before
    before
  • After
    demo

npm run serve:all로 띄우고 toastmark 새로 빌드해야 정상적으로 동작합니다ㅎㅎ

@js87zz js87zz force-pushed the feat/merged-table-with-toastmark branch from e7dff24 to e7b2374 Compare June 9, 2020 09:16
@js87zz js87zz merged commit cc54ec2 into master Jun 9, 2020
@js87zz js87zz deleted the feat/merged-table-with-toastmark branch June 9, 2020 09:17
@seonim-ryu seonim-ryu added this to the 2.2.0 milestone Jun 11, 2020
js87zz pushed a commit that referenced this pull request Jun 17, 2021
* feat: chart extension

* feat: multiple chart instance

* refactor: csv, dsv parser to papa

* refactor: apply review (ref #1010)

* fix: remove papa global dependency (fix #1011)

* refactor: change default chart to column

* feat: support whitespace separated values for chart data (ref #1009)

* refactor: apply code review (ref #1013)
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