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

Image import support #2

Closed
wants to merge 9 commits into from
Closed

Image import support #2

wants to merge 9 commits into from

Conversation

KubaZ
Copy link

@KubaZ KubaZ commented May 17, 2016

Removed custom parser and used remark + jsdom

import {stateFromElement} from 'draft-js-import-element';
import remark from 'remark';
import html from 'remark-html';
import {jsdom} from 'jsdom';
Copy link
Owner

Choose a reason for hiding this comment

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

I'm hesitant to pull in jsdom dependency since the browser natively does everything that jsdom does. But this is an interesting approach to go markdown -> ast -> html-string -> dom -> contentState. I'm curious to know what the downsides, if any are to double-parsing.

Copy link
Author

Choose a reason for hiding this comment

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

Moved to browser parser module from Your link. Currently i'm having some difficulties with blockquote test not passing, will need to look whats the issue

Copy link
Author

@KubaZ KubaZ May 18, 2016

Choose a reason for hiding this comment

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

Seems like issue with this test is connected to #1 and i think the problem is in stateFromElement module -> given blockquote markdown example: > test, remark translates it to <blockquote><p>test</p></blockquote> which is translated by stateFromElement to 'unstyled' type block with 'test' text, instead of 'blockquote' type.

@sstur
Copy link
Owner

sstur commented May 17, 2016

This is an interesting approach. I see you're doing markdown-string -> remark-ast -> html-string -> dom -> contentState. I'm hesitant to proceed with this due to double-parsing, but maybe that's not a huge deal given the wins of using mostly other people's well-tested code for the various conversions. If we do proceed with this approach, I'll certainly want to use the browser's native html parser instead of jsdom's. See here for an example.

I have been working on a dev branch that uses remark to parse the markdown to ast, but in my implementation I started writing custom code to go from remark-ast -> contentState

@KubaZ
Copy link
Author

KubaZ commented May 18, 2016

Since draft-js doesn't support nested blocks for elements other then lists - https://facebook.github.io/draft-js/docs/advanced-topics-nested-lists.html i will make temporary fix for blockquote by removing inner paragraph node

@KubaZ
Copy link
Author

KubaZ commented May 18, 2016

Added temp fix for blockquote. About this approach i think that using external, well tested code, outweights cons of double parsing. It makes code simple, short and easy to read / maintain :)

@Calyhre Calyhre mentioned this pull request Oct 10, 2016
@sstur
Copy link
Owner

sstur commented Jul 22, 2017

This is an old issue and has some conflicts. I'll close for now, but I'd be open for another attempt in the future.

@sstur sstur closed this Jul 22, 2017
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