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 svg-as-string mode #5

Closed
wants to merge 5 commits into from
Closed

Conversation

vhf
Copy link

@vhf vhf commented Jan 14, 2018

Here is a suggestion, I'd love some feedback.

Motivation: in some situations I'd rather output the SVG directly instead of serving it as an asset.

@brendo
Copy link
Contributor

brendo commented Jan 18, 2018

Hey! 👋

This is cool idea, I like it and sometimes we've wanted to do the same.

How do you feel about exposing the behaviour as a plugin option, rather than a vFile property?

Travis isn't happy with the build, I'll take a look at the underlying mermaid.cli and see if there's something that can be changed. Puppeteer recently went v1, so it may resolve the issue.

@vhf
Copy link
Author

vhf commented Jan 19, 2018

Agreed, it'd be better as a plugin option.

@vhf
Copy link
Author

vhf commented Jan 19, 2018

Done. 😄

@@ -5,7 +5,6 @@ module.exports = {
coveragePathIgnorePatterns: [
'/node_modules/',
],
mapCoverage: true,
Copy link
Author

Choose a reason for hiding this comment

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

@vhf
Copy link
Author

vhf commented Jul 22, 2018

@brendo Tests are still passing locally and still failing on Travis, I tried pushing updates for mermaid.cli (latest uses puppeteer 1.x), it didn't help. Any idea? :)

@vhf vhf force-pushed the render-svg-as-string branch 2 times, most recently from f0f31a4 to 64bf8fc Compare July 22, 2018 22:46
@artragis
Copy link

artragis commented Sep 5, 2018

Hello @brendo do you have some news about that?

@jimthedev
Copy link

Hey all, any update to this? If not would it be ok if someone picked it up? Wouldn't want to step on toes.

@vhf
Copy link
Author

vhf commented Nov 28, 2018

@jimthedev Feel free to take over my initial attempt!

@hugmanrique
Copy link

Any updates on this? I don't fully understand how puppeter works, so I'm not really able to pick this up and fix the test.

Copy link
Contributor

@brendo brendo left a comment

Choose a reason for hiding this comment

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

👋 I no longer work at Temando so can't help in terms of merging (@quangkhoa might be able to though?), but I've left a few comments.

@@ -90,7 +90,7 @@ function replaceLinkWithEmbedded(node, index, parent, vFile) {
* @param {boolean} isSimple
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, JSDoc update now that this an object that has two possible properties (there's a few other updates needed around this as well).

@@ -40,6 +41,17 @@ describe('remark-mermaid', () => {
expect(vfile.messages[0].message).toBe('mermaid code block replaced with graph');
});

it('can handle code blocks to svg string', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if the failing test is timing related. If this is pulled up before the previous "can handle code blocks" test, does this one pass and the old test fail?

@@ -30,15 +30,21 @@ function render(source, destination) {
// Clean up temporary file
fs.removeSync(mmdPath);

if (asString) {
const string = fs.readFileSync(svgPath, { encoding: 'utf-8' });
fs.removeSync(svgPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is the cause of the test errors 🤔

@hugmanrique
Copy link

Thank you for still helping on a project you don't own anymore ❤

@yuku
Copy link

yuku commented Feb 14, 2019

@brendo @quangkhoa Cloud you trigger CI again? I just forked this branch for debugging, and finally, I found that all tests pass now!

https://travis-ci.org/yuku/remark-mermaid/builds/493032690

EDIT: I simply pushed after checking out this branch. I changed nothing.

@brendo
Copy link
Contributor

brendo commented Feb 14, 2019

Sorry @yuku, I no longer work at Temando so don't have access to do so.

@zomble, @nfour or @zyrorl?

@vhf vhf closed this Apr 24, 2019
@remcohaszing
Copy link

Why was this PR closed?

From the comments I understand this PR would work if CI was triggered.

I’m still interested in this feature. If this PR needs more changes, please give some feedback.

@vhf
Copy link
Author

vhf commented May 10, 2019

@remcohaszing It's been 16 months since I opened this and I gave up, feel free to take over: #5 (comment) if you wish to push this forward, at this point I don't. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants