Skip to content

fix(markdown): do not add whitespaces between Latin and Hangul #5040

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.

Sign up for GitHub

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

Jump to bottom
Merged
ikatyang merged 9 commits into from
Sep 2, 2018
Merged

fix(markdown): do not add whitespaces between Latin and Hangul #5040

ikatyang merged 9 commits into from
Sep 2, 2018

Conversation

@ikatyang
Copy link
Member

@ikatyang ikatyang commented Sep 1, 2018 •
edited
Loading

Fixes #5028

Based on #5028 (comment), it seems Korean text uses conventional space so we're not going to add whitespaces between Latin and Hangul.

Prettier pr-5040
Playground link

--parser markdown

Input:

안녕하세요. 저는 Prettier을 굉장히 잘 사용하고 있습니다.
그런데 Prettier는 영어와 한글 사이에 무조건 빈 공간을 입력하고 있어요.
... ㅠㅠ

Output:

안녕하세요. 저는 Prettier을 굉장히 잘 사용하고 있습니다.
그런데 Prettier는 영어와 한글 사이에 무조건 빈 공간을 입력하고 있어요.
... ㅠㅠ
  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • I’ve read the contributing guidelines.

suchipi
suchipi reviewed Sep 1, 2018
View reviewed changes
scripts/build/rollup-plugins/evaluate.js
}

const json = JSON.stringify(
require(id.replace(/^\0commonjs-proxy:/, "")),
Copy link
Member

@suchipi suchipi Sep 1, 2018

Choose a reason for hiding this comment

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

Where does this \0commonjs-proxy: come from?

Copy link
Member Author

@ikatyang ikatyang Sep 1, 2018

Choose a reason for hiding this comment

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

rollup-plugin-commonjs I guess?

Copy link
Member

@suchipi suchipi Sep 1, 2018

Choose a reason for hiding this comment

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

Ah okay, I was just confused

j-f1
j-f1 reviewed Sep 1, 2018
View reviewed changes
tests/markdown_splitCjkText/__snapshots__/jsfmt.spec.js.snap Outdated
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`cjk.md - markdown-verify 1`] = `
exports[`cj.md - markdown-verify 1`] = `
Copy link
Member

@j-f1 j-f1 Sep 1, 2018

Choose a reason for hiding this comment

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

What do you think about renaming these to chinese-japanese.md and korean.md so it’s clearer at first glance what they’re testing?

Or are cj and k common abbreviations?

Copy link
Member

@suchipi suchipi Sep 1, 2018

Choose a reason for hiding this comment

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

I've seen CJK, but never CJ and K as separate abbreviations

@ikatyang ikatyang merged commit 9120689 into prettier:master Sep 2, 2018
@ikatyang ikatyang deleted the fix/markdown/korean-whitespace branch September 2, 2018 08:20
@ikatyang ikatyang added this to the 1.15 milestone Oct 25, 2018
@christophercr christophercr mentioned this pull request Nov 13, 2018
Closed
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@suchipi suchipi suchipi approved these changes

@j-f1 j-f1 j-f1 left review comments

Assignees

No one assigned

Labels

locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Projects

None yet

Milestone

1.15

Development

Successfully merging this pull request may close these issues.

3 participants

@ikatyang @suchipi @j-f1