[REFACTOR] rework library global options

tbouffard commented 3 years ago

Current definition

The options to pass to the constructor are as in the following example

{
  mouseNavigationSupport: true,
  zoomConfiguration: {
    throttleDelay: 30,
    debounceDelay: 40,
  }
}

The HTML element used as a container to render the BPMN Diagram is not part of the options and must be passed

as a dedicated constructor parameter (not very consistent with setting the other elements)
as an HTMLElement which forces the application instantiating the lib to prior call document.getElementById or something similar. Various discussions on that topic previously happened about how this is painful or not readable, see https://github.com/process-analytics/bpmn-visualization-examples/pull/99#pullrequestreview-536462930 or https://github.com/process-analytics/bpmn-visualization-js/pull/892#pullrequestreview-536345810

Typical constructor call with the current implementation:

new BpmnVisualization(
    document.getElementById('bpmn-container'),
    { mouseNavigationSupport: true } // generally, no need to tune the zoom configuration
);

Proposed changes

HTML container

Allow to provide it as a string (id of the element in the HTML page) or as an HTMLElement (as today) Having it configured with other lib options will allow us to have a constructor with a single argument. All configuration options will be consistent, this will also avoid changing the constructor signature in the future. This is the way the G6Js diagramming lib is configured, see https://g6.antv.vision/en/docs/manual/getting-started#step-3-instantiate-the-graph

const graph = new G6.Graph({
  container: 'mountNode', // String | HTMLElement, required, the id of DOM element or an HTML node
  width: 800, // Number, required, the width of the graph
  height: 500, // Number, required, the height of the graph
});

We could eventually allow to pass the container as a single parameter of the constructor to avoid breaking existing example and avoid passing an object for very simple settings. Does it worth the effort?

Navigation

Discussions started on https://github.com/process-analytics/bpmn-visualization-js/pull/865#discussion_r518650374 We should have a dedicated property that hold all navigation options, to group them all togheter and let easily add new settings on that topic The mouseNavigationSupport name is too restrictive: navigation is also (at least partially for now) available with gestures/pinch/tap so putting a reference to the mouse in the name tends to state that some elements are not supported.

Putting configuration in the zoom settings name is redundant as we already know that we are settings configurations/options here.

Example

{
  container: 'bpmn-container',
  navigation: {
    enabled: true,
    zoom: {
      throttleDelay: 30,
      debounceDelay: 40,
    }
  }
}

How other libs manage this

gitgraphjs: https://gitgraphjs.com/#3

const graphContainer = document.getElementById("gitgraph");
const gitgraph = createGitgraph(graphContainer);

graph-js: pass an element id - https://github.com/feds01/graph-js

// where elementId is the given id of the div you wish for the graph to use
let graph = new Graph("elementId", {
  title: "A new graph!",
  x_label: "X-Axis",
  y_label: "Y-Axis"
});

chart-js: pass an html context element - https://www.chartjs.org/docs/latest/

<canvas id="myChart" width="400" height="400"></canvas>
<script>
var ctx = document.getElementById('myChart').getContext('2d');
var myChart = new Chart(ctx, {....

gojs: http://flowdiagram.itstack.org/learn/index.html In JavaScript code you pass the div's id when making a Diagram:

var $ = go.GraphObject.make;
var myDiagram =   $(go.Diagram, "myDiagramDiv");

aibcmars commented 3 years ago

Concerning previous discussions, I agree with the following example as a constructor parameter:

{
  container: 'bpmn-container',
  navigation: {
    enabled: true,
    zoom: {
      throttleDelay: 30,
      debounceDelay: 40,
    }
  }
}

csouchet commented 3 years ago

I agree with the propostion 🙂

process-analytics / bpmn-visualization-js