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

#422 use foreignObject/div to place actor label in sequence diagram #423

Merged

Conversation

whyzdev
Copy link

@whyzdev whyzdev commented Dec 1, 2016

re #422, the change to foreignObject/div is enabled by config.textPlacement: fo. Otherwise, use svg text as before without foreignObject.

_drawTextCandidateFunc may be generalized and used elsewhere in sequence diagram and beyond. there is opportunity to consolidate toward to some common code for text placement.

pased: npm run tape && npm run karma && npm run dist
npm run jasmine was failing unrelated to this change.
manually tested:

   sequenceDiagram
    Alice->>John: Hello John, how are you?
    John-->>Alice: Great!
    very very very long long very long ->> John: ok?

the change may go further to consolidate
\n support.
please let me know if it's better keep going with in this PR, or new PR after the current changeset is satisfied and merged.

@whyzdev
Copy link
Author

whyzdev commented Dec 1, 2016

Please review, but don't merge yet. When config (sequenceDiagram not sequenceConfig) textPlacement is not 'fo', it's not rendering as before. Will fix and check both firefox and chrome some version.

Also saw some other html label config but not used in sequence diagram. will see if/how can consolidate. And some doc out of sync...

Let me know anything else. Thanks,

@knsv
Copy link
Collaborator

knsv commented Dec 2, 2016

Much appreciated. Will merge when the build is completed. Seems to take a lot of time, travis might need some configuration. Will try to make a release in dec with the latest changes.

@knsv knsv merged commit 4984f9e into mermaid-js:master Dec 2, 2016
@whyzdev
Copy link
Author

whyzdev commented Dec 2, 2016

Thanks. np, love doing it. Let's see impact to the users.
I am going to continue looking into refactor/consolidate text placement code in sequence diagram and others. Try textwrap attribute as in the existing drawText method used by note etc. Let's keep #422 open.

btw, what do we say about deleting commented and unused code? will go along and see in upcoming PR. Thanks

@whyzdev whyzdev deleted the issue422_seq_actor_text_placement_by_fo branch December 10, 2016 20:38
mgenereu pushed a commit to mgenereu/mermaid that referenced this pull request Jun 25, 2022
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