Update to latest version of Slate #36
Conversation
|
Hey, thanks for the work – I'll definitely be upgrading Slate at some point but tend to do it in batches as there are too many breaking changes to continuously keep up. The paragraph tags are definitely there purposefully to fix earlier bugs so this will be difficult to merge with that removal. I'd recommend you leave them in and adjust you CSS to account for them, but up to you. |
|
I'm currently working on some changes which will hopefully fix #35. Once that's done and stable, I'll revisit the paragraph tags and, at that point, we can discuss whether this can be merged or not. In the meantime, I'll be publishing my fork as |
|
|
|
@tommoor Any reason this can't be merged? FWIW, it'd be really helpful for me as This PR does fix some issues with nested bold/italic marks and some other list item issues. |
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "slate-md-serializer", | |||
| "version": "5.3.3", | |||
| "name": "@edithq/slate-md-serializer", | |||
There was a problem hiding this comment.
These changes should probably be reverted, correct?
| @@ -10,36 +10,62 @@ function getNodes(text) { | |||
| return reparsed.document.nodes; | |||
| } | |||
|
|
|||
| // Asserts whether the original Markdown string is the same as the one created | |||
| // by parsing and re-rendering the original. | |||
| function assertSymmetry(text, value) { | |||
There was a problem hiding this comment.
This seems like it functionality performs the same job as getNodes?
There was a problem hiding this comment.
Agreed, I think the snapshot approach is preferable as it will make any regressions or bugs easier to identify and fix. It also makes changes to the renderer output trackable in the git history.
|
@Aerlinger unfortunately there seems to be too many unrelated changes in this PR to merge cleanly. It also changes the paragraph handling as commented previously. If the fork works for you, I'd definitely encourage you to use it until this repo is updated |
| @@ -1148,7 +1148,7 @@ Parser.prototype.tok = function() { | |||
| while (this.next().type !== "list_item_end") { | |||
| body.push( | |||
| this.token.type === "text" | |||
| ? this.renderer.paragraph(this.inline.parse(this.token.text)) | |||
There was a problem hiding this comment.
What was the reason for this change? Is it necessary for Slate 0.47? It will likely cause issues per @tommoor's comments.
src/__tests__/renderer-test.js
Outdated
| }); | ||
|
|
||
| test("ignores empty link", () => { | ||
| const text = `[empty]()`; | ||
| expect(getNodes(text)).toMatchSnapshot(); | ||
| assertSymmetry(text, false); |
There was a problem hiding this comment.
How is it possible that both this statement and the previous one can pass the test?
|
@tommoor I see. Out of curiosity, why is the paragraph rendering necessary? Is it for legacy reasons with how the original source was written or is it for compatibility with Slate? Wouldn't it be simpler just to render text leaves instead of wrapping them in a paragraph if it's not necessary? |
Update deps & change handling of empty paragraphs & lists
|
@eduardoboucas I have stumbled upon this repository, as I was looking for a Slate MD serializer, and now it turns out, this repo supports only earlier versions of Slate. I have also seen that '@edithq/slate-md-serializer' is also available on npm, but I have installed it and it does not seem to work. Should it actually work at this point? |
|
@DominikLevitsky it should work, yes. What's failing for you? |
|
It would be great to get this merged and released. Is there any idea of a timeline? |
Escape backticks in code blocks
Hi all.
I've recently started using Slate and came across this Markdown serialiser. After reading #27, I learned that this fork is not compatible with the latest version (0.47), so I had a go at making the necessary changes, which are included in this PR.
The main change was to remove the
leavesproperty fromtextobjects. Slate was throwing deprecation warnings before, which this PR fixes.I've also made a change to how list items are processed (master...edithq:master#diff-6947033678b93d106e25614dd972e66fL1151), so that list items are not wrapped inside a paragraph node. I'm not sure this is a result of any changes in Slate core, but the reality is that my list items were not being rendered properly and this seems to fix it.
@tommoor I assume you're probably not interested in merging this at this point, because from what I gather you're pegged to an older version of Slate, but if you could kindly glance over this PR and let me know if there's something blatantly wrong in the implementation, that would be much appreciated.
If you're happy with it, perhaps we can leave the PR open and we can merge our forks again once you're happy to upgrade.
Thanks!