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

[feature] expose inline level renderer method tag #962

Conversation

moreJs
Copy link

@moreJs moreJs commented Dec 3, 2017

as user, we need expose tag inline renderer method to process some user-side logic.

@moreJs
Copy link
Author

moreJs commented Dec 3, 2017

@joshbruce

@joshbruce
Copy link
Member

@moreJs: Please write the associated spec cases for what's being accomplished by this code.

@moreJs
Copy link
Author

moreJs commented Dec 4, 2017

spec cases:
this code expose inline level rendered method tag, so if you want to do some special logic with html tag, you can do this in rendered method tag.

@joshbruce
Copy link
Member

@moreJs: Sorry. I mean please write the actual tests in the project files demonstrating what the PR does, how consumers of the library use that functionality, and that the code works as designed.

Then update the PR.

@moreJs
Copy link
Author

moreJs commented Dec 4, 2017

@joshbruce i got it.

@moreJs
Copy link
Author

moreJs commented Dec 4, 2017

@joshbruce i just expose inline renderer method tag(text:string) to library consumers, and not add any new feature about mardkown render, just like #950 . so i am confused about how to write tests case in current architecture.

@joshbruce
Copy link
Member

joshbruce commented Dec 6, 2017

@moreJs: Good point. And, again, I do apologize for not being as familiar with the library as others may be.

Having said that, what exactly are we exposing? I know it says "tag," but tag is not an HTML element.

I can't tell what's being returned exactly from an HTML perspective, nor what input would be required from a markdown perspective. Therefore, I'm not sure if exposing this feature would be sticking to the two specifications we are actively supporting (listed in the recently updated #956).

So, maybe a test that shows how this changes the usage of marked from an implementation perspective:

marked.tag(...)

// result ...

let markdown = "...";
markdown.toHtml(markdown)

// result ...

Is that making sense. Maybe @UziTech can help us?

@Feder1co5oave
Copy link
Contributor

Renderers are to convert abstract syntax elements into html. Since the tag element is already html, there's not point in requiring a renderer for it.

I guess you'd like to use this to implement some feature on your own, but it just doesn't make sense for the library itself.

@joshbruce joshbruce added this to the 0.5.0 - Architecture and extensibility milestone Dec 27, 2017
@joshbruce
Copy link
Member

Closing as merge conflicts, missing tests. If we were to do something like this I think it would be in 0.5.0 milestone as well. Given that we've, hopefully, taken care of the XSS problems, we're really trying to focus on fixing defects that cause Marked not to be copliant with the specs in #956. Unfortunately, that is going to mean closing PRs that don't fit the guidelines; the main one being a test case that demonstrates how something would used "in the wild".

@joshbruce joshbruce closed this Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants