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: keep empty line between lists #2598

Merged
merged 5 commits into from
Jul 12, 2022

Conversation

jajugoguma
Copy link
Contributor

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

  • Fixed an issue that removes empty line between lists when convert to markdown.
    • If there is an empty line, it keeps to <br>.

As-Is

To-Be


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

Copy link

@adhrinae adhrinae left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다. 맥락이 부족하여 ToMdConvertorState 클래스 내부까지만 좀 살펴봤는데, 코멘트 남겨주신 부분 확인하고 변경 가능하면 변경해주시는게 좋을 것 같아요.

apps/editor/src/__test__/unit/convertor.spec.ts Outdated Show resolved Hide resolved
Comment on lines +87 to +92
const prevDelim = state.getDelim();

state.setDelim('');
state.write('<br>');

state.setDelim(prevDelim);

Choose a reason for hiding this comment

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

동작이 wrapBlock 이라는 메서드와 유사해 보이네요.

wrapBlock(delim: string, firstDelim: string | null, node: Node, fn: () => void) {
const old = this.delim;
this.write(firstDelim || delim);
this.delim += delim;
fn();
this.delim = old;
this.closeBlock(node);
}

손쉽게 외부에서도 delim을 다루기 위해 메서드를 만든 것 까진 좋은데, ToMdConvertorState 클래스 내부에서는
setDelim 메서드를 쓰지 않고 this.delim 에 할당을 하는 동작이 마구 일어나고 있으니 코드에 일관성이 없어 보여요.

Choose a reason for hiding this comment

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

내용이 두개로 나뉘어야했는데 리뷰를 읽다가 하나로 뭉쳐졌나보네요.

  1. 지금 작성한 부분은 wrapBlock 으로 대체가 안되는지
  2. get/setDelim 메서드가 만들어졌으니 this.delim 을 다룰 때 해당 클래스의 모든 연관 동작이 수정되어야 하지 않느냐

위 두개는 다른 맥락이었어요.

무튼 리뷰 반영된 코드에는 wrapBlockget/setDelim 만 적용하고 있는데, flushClose, write 메서드도 같은 변경이 필요해요.

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.

리뷰 완료합니다.

@jajugoguma jajugoguma merged commit 78e2184 into master Jul 12, 2022
@jajugoguma jajugoguma deleted the fix/keep-empty-line-between-lists branch July 12, 2022 06:25
ahamelers pushed a commit to ahamelers/tui.editor that referenced this pull request Aug 21, 2023
* fix: keep empty line between lists

* fix: remove delimiter when add empty line between list items

* test: add test for convertor

* chore: apply code reviews
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.

5 participants