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

Table blocks should be surround by newline characters #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olipo186
Copy link

Hi!

This is an improved version of a previous PR that fixes the table syntax issue described here: #40

  • Updated renderer to surround table blocks with newline characters
  • Updated parser to interpret surrounding newline characters (if present) as part of the table block syntax to avoid the creation of excess paragraph nodes
  • The parser previously skipped the last cell in a table if it was empty (caused by the regexp cap[3].replace(/(?: *\| *)?\n$/, "")). Fixed this and updated test to expect all cells (even empty) to be properly parsed.

… should ignore surrounding newline characters when present (retains backwards compatibility). Parser should not ignore empty table cells.
@olipo186
Copy link
Author

Hi @tommoor!

I had another look at this issue and dug into your unit tests this time. I'm hoping this PR will be merged. It will make the markdown produced by your editor a little bit more similar to other libs which will improve interoperability.

@tommoor
Copy link
Owner

tommoor commented Jan 27, 2020

Hi @olipo186 - I can definitely agree with the sentiment that the tables produced should be parsable by other markdown implementations.

It looks like the snapshots have changed in this branch which means the Slate output isn't compatible with the previous version though?

Sidenote: You should know that I'm currently working on converting rich-markdown-editor to Prosemirror which will largely make this redundant unless someone chooses to fork and continue development on the Slate version

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