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

Add markdown rendering demo #82

Closed
wants to merge 2 commits into from
Closed

Add markdown rendering demo #82

wants to merge 2 commits into from

Conversation

therustmonk
Copy link
Member

@therustmonk therustmonk commented Jan 5, 2018

This changes is an attempt to resolve #67
Also this PR add oninput event support to <textarea>.

Cargo.toml Outdated
stdweb = "0.3"
#stdweb = "0.3"

stdweb = { path = "../stdweb" }
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, in your .cargo/config in your home directory you can add this:

paths = [
    "/path/with/your/local/checkout/stdweb"
]

And then cargo will use your local copy instead of the upstream one without you having to modify the Cargo.toml.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was a little hasty about this change ) Great advice! And really much easier! 👍

@therustmonk therustmonk mentioned this pull request Jan 5, 2018
@therustmonk therustmonk changed the title Add description field to crm demo Add markdown rendering demo Jan 5, 2018
@therustmonk therustmonk mentioned this pull request Jan 30, 2018
8 tasks
Copy link
Contributor

@vitiral vitiral left a comment

Choose a reason for hiding this comment

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

Really excited for this, definitely a requirement for Artifact 👍

@@ -0,0 +1,104 @@
/// Original author of this code is [Nathan Ringo](https://github.com/remexre)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't you think we should move this into a separate crate?

Copy link

Choose a reason for hiding this comment

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

While I think this functionality is worth of it's own crate. I also believe that having it as part of the examples would make it easier for new people to the library (like me) to understand how to generate new dom elements that can be used within the html! macro.

So I do see the value of it in the examples. Is there any other place I can look for such documentation without having to dig into the implementation code?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a good point! It can be made it's own crate separately.

} else if let Ok::<TextAreaElement,_>(textarea) = this.clone().try_into() {
textarea.value()
} else {
panic!("only InputElement or TextAreaElement could have oninput event listener");
Copy link
Contributor

Choose a reason for hiding this comment

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

technically this should be unreachable! right?

@@ -263,6 +263,19 @@ impl<MSG> VTag<MSG> {
// IMPORTANT! This parameters have to be set every time
// to prevent strange behaviour in browser when DOM changed
set_checked(&input, self.checked);
Copy link
Contributor

@vitiral vitiral Mar 21, 2018

Choose a reason for hiding this comment

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

do you think set_checked should be called for TextAreaElement as well?

Edit: probably not -- checked is for checkboxes associated with the overloaded <input> element.

@vitiral
Copy link
Contributor

vitiral commented Apr 26, 2018

is there anything you need to move this forward?

I plan on using markdown + textarea elements in artifact very soon. I will probably be merging this branch locally and making any bugfixes there. Do you mind if I take over this PR?

@vitiral
Copy link
Contributor

vitiral commented Apr 26, 2018

It looks like the TextAreaElement piece of this was already merged, correct? So it is just the markdown example remaining?

@hgzimmerman
Copy link
Member

@vitiral I have a working implementation of markdown -> HTML rendering based on a more recent version of Yew. I can't guarantee that it works with Yew 0.4.x at this very moment, but if you want a reasonable place to start, then this file may be of use.
Its based on updated code from the original author that inspired this PR, the source of which can be found here.

I don't have time to work this into an example at the moment, so I encourage you to take the lead on this if you wish.

@therustmonk
Copy link
Member Author

@vitiral @hgzimmerman I would really appreciate it if you could help me with that. I haven't enough time to complete it, because I want to stabilize the latest version of the framework.
If you have own implementation I could close this PR in favour of your. There is no special requirements to features. The main idea to show how to build own tree from scratch.

What about to create a separate crate for rendering markdown? Is this in demand? Or example is enough?

@vitiral
Copy link
Contributor

vitiral commented May 2, 2018

I could not get pulldown_cmark to compile in wasm or emscripten so I just included the commonmark javascript library and used the js! macro to get the html, then Node::from_html and VRef::Node to insert it into my webapp. Works like a charm and is good enough for now IMO!

If you could get pulldown_cmark to compile you could still just use Node::from_html for now if you didn't want to do anything special (which you shouldn't be... it's markdown. What special stuff I do I do by replacing specialized markdown syntax with html).

@deniskolodin everything I need works great for me regarding this issue. I say feel free to close. I think a yew_markdown crate should totally happen, but I don't think the yew examples should necessarily include its implementation. Also, the ecosystem is a bit too unstable at the moment to have external dependencies like pulldown_cmark (these are just my opinions).

@hgzimmerman
Copy link
Member

hgzimmerman commented May 7, 2018

Sad to hear that you couldn't get pulldown_cmark to work, but glad to hear you found an alternative solution. I'm using a version of pulldown_cmark that points to a specific commit, likely from a while ago. A more recent version of that crate may not play well with WASM/JS targets.

I think there is demand for a yew-markdown crate, but given the current reliance on a non-published version of pulldown_cmark, it doesn't seem wise to invest the time in a separate crate before a known good WASM/JS compatible markdown parser is available. I think having an example for the moment is more than enough, but I will spend a little time investigating pulldown_cmark and other markdown parsing options in regards to web-compatibility.

I anticipate having some free time a week from now and will allocate some time towards getting an example built then.

@Istar-Eldritch
Copy link

That is strange to hear @vitiral I have a working version of pulldown_cmark pointing to the latest master branch here: https://github.com/Istar-Eldritch/personal-page/blob/master/src/markdown.rs

It's basically the same implementation you have here with slight moditifcations.

@hgzimmerman
Copy link
Member

Hey, I just want to let you know that I haven't forgotten about this. In light of the forthcoming significant changes to the framework, I'm going to wait to open a PR for markdown rendering until your changes land.

@hgzimmerman
Copy link
Member

#292 should now take precedence over this PR.

@therustmonk
Copy link
Member Author

Closed in favour #292! Thank goes to @hgzimmerman 🎉

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.

Change tag type in VDom
5 participants