bpmn-io / bpmn-js

A BPMN 2.0 rendering toolkit and web modeler.
https://bpmn.io/toolkit/bpmn-js/
Other
8.55k stars 1.32k forks source link

BPMN Renderer: functions should be part of bpmn renderer instance #1347

Open 0x00000001A opened 4 years ago

0x00000001A commented 4 years ago

Is your feature request related to a problem? Please describe.

I want to extend BPMN renderer functions renderLabel / renderEmbeddedLabel because we want to show translated names (different from the businessObject.name) in the BPMN elements. Now, I need to copy handlers and a lot of other stuff from the BpmnRenderer.js to my custom renderer.

Describe the solution you'd like

I want to be able to override only functions specified above (renderLabel / renderEmbeddedLabel) for customizations that I need.

Describe alternatives you've considered

Copy-paste from sources a lot of code.

Additional context

Currently, these functions (renderLabel / renderEmbeddedLabel) are just placed in the scope of BpmnRenderer, but they are not part of it. It creates some problems for overriding. They are should be part of the BpmnRenderer prototype like drawShape/drawConnection/etc functions.

pinussilvestrus commented 4 years ago

Thanks for open the issue, I guess that's a very valid use case in terms of creating a more customizable Renderer. We're open for contributions which improve the situation 👍 Feel free to open a PR if you want to 🚀

philippfromme commented 4 years ago

I'd like to understand your use case better before we dive into the code. You want to translate diagrams, right? Have you considered translating them based on the XML instead? How is that translation supposed to work? Let's say I import a an English diagram, I translate it to German. Then I create a new element and give it a name. What language am I supposed to use? Will it be translated immediately?

0x00000001A commented 4 years ago

I'd like to understand your use case better before we dive into the code. You want to translate diagrams, right? Have you considered translating them based on the XML instead? How is that translation supposed to work? Let's say I import a an English diagram, I translate it to German. Then I create a new element and give it a name. What language am I supposed to use? Will it be translated immediately?

We have a multi-language application. So, the user can switch to another language at any time. When the user chooses the language, new translations will be loaded dynamically on the background. Then, when the user creates a new element, we provide him multi-language input, where he can fill the name for various languages. Another use-case for our company is that we are not allowing the user to edit the name for some elements. Instead, we provide some special name that was generated-by-server. So, businessObject.name and rendered label may be different things in our case. In short, we will store some, let's say identifier (and not only for translations) in the businessObject.name and render some label, based on this value.

Another point of view, that has a place - is that it's strange to keep functions, that are part of logic, in this case - part of the renderer, outside of this and/or prototype. It's just a matter of time when some users/companies will need to override some behavior. There a lot of companies, with a lot of really specific requirements (and you may not get/understand them all). And, in my opinion, it's good to let them override the behavior of some specific functions, instead of copy-pasting of the entire module.

At the same time, I wonder, why they're a lot of different approaches was used. In some modules (e.g. TextRenderer) everything is placed in this, in other modules - partially in this, partially in prototype and some are not allowed to extend (w/o reason). I guess you like the idea, that you can override diagram-js functions like canResize in the AutoResizeProvider, without explanations about why you need it, then why you are not allowing us to do the same thing? Why so different design concepts used w/o any visible reason.

nikku commented 3 years ago

Thanks for clarifying @0x00000001A.

I'll have a look into your PR. It definitely makes sense to expose these methods in a way that they can be extended / overridden. Let's see what you proposed there :smiley:.