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 BPMN support #178

Merged
merged 1 commit into from
May 1, 2020
Merged

Add BPMN support #178

merged 1 commit into from
May 1, 2020

Conversation

nekator
Copy link
Contributor

@nekator nekator commented Apr 24, 2020

add support to render BPMN diagram via bpmn-js
GH-44

@ggrossetie
Copy link
Member

Thank you very much @nekator
It looks great! 👍

I will do a more in depth review this week.

@nekator
Copy link
Contributor Author

nekator commented Apr 26, 2020

You are welcome. Looking forward for your feedback. 😀

@ggrossetie
Copy link
Member

Sorry I didn't have time to give it a try yet but since it's a new container we will need to update the following pages:

@nekator
Copy link
Contributor Author

nekator commented Apr 29, 2020

I'll take a look into the docs and update the relevant parts today or tomorrow.

@nekator
Copy link
Contributor Author

nekator commented Apr 29, 2020

I updated the docs for install and architecture. I also changed the caching version to the diagram version.

bpmn/package.json Outdated Show resolved Hide resolved
bpmn/Dockerfile Outdated Show resolved Hide resolved
@ggrossetie
Copy link
Member

Overall it's looking very good, thanks @nekator
I left a few comments but I can take care of them if you want.

I wonder if we should remove the XML header from the SVG. Currently, bpmn-js will generate the following SVG:

<?xml version="1.0" encoding="utf-8"?>
<!-- created with bpmn-js / http://bpmn.io -->
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="856" height="587" viewBox="150 75 856 587" version="1.1">
<!-- ... -->

Maybe it can be configured...

@nekator
Copy link
Contributor Author

nekator commented Apr 29, 2020

I will and the relevant changes tomorrow.

Regarding XML header i would suggest to keep it.

Maybe we should make it configurable to support other output formats because bpmn-js supports multiple formats.

add support to render BPMN diagram via bpmn-js
yuzutechGH-44

changes from code review
@nekator
Copy link
Contributor Author

nekator commented Apr 30, 2020

Resolved everything and squashed it to a single feature commit.

@ggrossetie
Copy link
Member

Regarding XML header i would suggest to keep it.

I don't have a strong opinion, so we will keep it for now.

Maybe we should make it configurable to support other output formats because bpmn-js supports multiple formats.

Oh nice, I didn't know!
@nekator I guess that since they are using a <canvas> we can export it to png using toDataURL. Is that what you had in mind?
But as far as I understand, it's only possible to save as SVG using the Viewer API (ie. BaseViewer.prototype.saveSVG) right?

I think we can add additional output formats in follow-up pull requests. Wdyt?

Copy link
Member

@ggrossetie ggrossetie left a comment

Choose a reason for hiding this comment

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

Code is looking really good. Well done @nekator 👍

@nekator
Copy link
Contributor Author

nekator commented Apr 30, 2020

Adding additional formats would be another PR IMO. For now it is good to have BPMN diagram to svg. I will take a look into other formats maybe this weekend.

I'm glad you like the code and that you took the time to review it properly. Thank you 😊

@ggrossetie ggrossetie merged commit f90c762 into yuzutech:master May 1, 2020
@nekator nekator deleted the bpmn-support branch May 1, 2020 16:28
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