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

#478 API crashes on 2nd render() call #994

Merged
merged 2 commits into from
Oct 15, 2019
Merged

Conversation

KoljaTM
Copy link

@KoljaTM KoljaTM commented Oct 15, 2019

Resolves #478

Remove element from DOM before rendering to avoid conflicts in case of rerendering

@coveralls
Copy link

coveralls commented Oct 15, 2019

Pull Request Test Coverage Report for Build 1103

  • 0 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 52.813%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/mermaidAPI.js 0 3 0.0%
Totals Coverage Status
Change from base Build 1097: -0.03%
Covered Lines: 3067
Relevant Lines: 5715

💛 - Coveralls

@IOrlandoni
Copy link
Member

Is there any way we could set up a test for this? Either unit or e2e...

@KoljaTM
Copy link
Author

KoljaTM commented Oct 15, 2019

I have looked a bit into it, the problem is that we don't have the real document object at hand and therefore the d3.select(...).node() function doesn't exist. At the moment, there don't seem to be tests for the render(), at least no unit-tests. I can give it another look and if unit doesn't work out, add something to the e2e tests.

@IOrlandoni IOrlandoni changed the base branch from master to develop October 15, 2019 13:16
@IOrlandoni IOrlandoni self-requested a review October 15, 2019 13:21
@IOrlandoni
Copy link
Member

Pending confirmation of the person who had the issue assigned to them.

@IOrlandoni IOrlandoni requested review from klemmchr and removed request for IOrlandoni October 15, 2019 13:26
@klemmchr
Copy link
Member

Your pr includes a commit from knsv about font configuration stuff, could you rebase your branch/cherrypick the relevant commit and create a new branch from develop? knsv was that nice to write a CONTRIBUTING.md recently, you might want to have a look into it.
TL;DR this will be merged to develop

@KoljaTM
Copy link
Author

KoljaTM commented Oct 15, 2019

OK sure, no problem

@KoljaTM
Copy link
Author

KoljaTM commented Oct 15, 2019

I added an e2e test now. It works (and fails when I take my change out). It seems that the width of the graph is not determined correctly though. No idea where that comes from, but it doesn't seem related to this bug.
image

@klemmchr
Copy link
Member

klemmchr commented Oct 15, 2019

This could be related to the commit of knsv regarding font size configuration that you included in this pr. It needs to be removed from this pr.

- remove element from DOM before rendering to avoid conflicts in case of rerendering
- add e2e test for (re)rendering by api
@knsv
Copy link
Collaborator

knsv commented Oct 15, 2019

The issue with the clipped text has to do with different styling applied during rendering compared with font applied when the graph is put in to place after rendering. The font-family fix should handle that problem, and has just been merged to develop. I have reviewed this and will merge it.

My only concern is if there are some use-case where this might break some integration...

@klemmchr
Copy link
Member

There might be cases that rely on the current behavior but I would consider that as a misuse of the functionality.

@knsv
Copy link
Collaborator

knsv commented Oct 15, 2019

Merging but it will not be inkclud in 8.4.0. next release though.

@knsv knsv merged commit 0ce42e6 into mermaid-js:develop Oct 15, 2019
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.

None yet

6 participants