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

Update development/v0.1.4 branch with diagram creation using textual representation #336

Merged
merged 34 commits into from
Apr 6, 2017

Conversation

chuajiaxuan
Copy link
Contributor

No description provided.

@amoshydra
Copy link
Contributor

I will do a manual merging with dev/encapsulate-viewer-with-iframe and submit a pull-request together.
Putting content into iframe will change how some of the library behave.

@amoshydra amoshydra closed this Apr 6, 2017
@amoshydra
Copy link
Contributor

This pull-request is closed. Do not delete the branch yet.

@amoshydra amoshydra reopened this Apr 6, 2017
@amoshydra
Copy link
Contributor

amoshydra commented Apr 6, 2017

Manual merging is not necessary, using iframe for rendering does not change any behaviour.
We can merge this after reviewing. Reopening this pull-request

Copy link
Contributor

@amoshydra amoshydra left a comment

Choose a reason for hiding this comment

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

Looks good.
Added 2 suggestions for this pull-request. You may review the suggestion and merge the pr.

export default function setup(md, options) {
const defaultRender = md.renderer.rules.fence;

/* eslint no-undef: 0 */
md.renderer.rules.fence = (tokens, idx, opts, env, self) => {
const token = tokens[idx];
token.content = filter(token.content);
if (token.info === 'sequence') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like switch case could be beneficial here to remove the need of repeating <pre class="sequence">${token.content}</pre>;

You can consider doing this:

switch (token.info) {
  case 'sequence':
  case 'flow':
  case 'mermaid':
  case 'graphviz':  // fallthrought
    return `<pre class="${token.info}">${token.content}</pre>`;
  default: {
    // pass token to default renderer.
    return defaultRender(tokens, idx, opts, env, self);
  }
}

Copy link
Contributor Author

@chuajiaxuan chuajiaxuan Apr 6, 2017

Choose a reason for hiding this comment

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

Thanks! You gave me another idea! I store the accepted diagrams library in an array and do this:

if (diagramsLib.includes(token.info)) {
      return `<pre class="${token.info}">${token.content}</pre>`;
}

const diagram = Diagram.parse(content);
seqDiagrams[i].innerHTML = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Using innerHTML may not be a good practice here.
In the future, you may consider manipulating the DOM using api like "appendChild", "removeChild" etc.

See:

In this case, you may consider using a for loop to remove all the children inside seqDiagrams[i]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yes yes. I have read about this before! Will go change it! :)

@chuajiaxuan chuajiaxuan merged commit 266c98f into development/v0.1.4 Apr 6, 2017
@chuajiaxuan chuajiaxuan deleted the dev/merge-diagramsRenderer branch April 6, 2017 11:33
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