Implements default gateway flow#1184
Conversation
| this.defaultFlow = this.node.definition.sourceRef | ||
| && this.node.definition.sourceRef.default | ||
| && this.node.definition.sourceRef.default.id === this.node.definition.id; |
There was a problem hiding this comment.
Used 3 times. Could become a method.
isDefaultFlow() {
return this.node.definition.sourceRef
&& this.node.definition.sourceRef.default
&& this.node.definition.sourceRef.default.id === this.node.definition.id;
}
Co-Authored-By: Lisa Fehr <lisa-fehr@users.noreply.github.com>
johnblackspear
left a comment
johnblackspear
left a comment
There was a problem hiding this comment.
Left a few comments :)
src/components/modeler/Modeler.vue
Outdated
| }, | ||
| }, | ||
| methods: { | ||
| defaultFlow(flow) { |
There was a problem hiding this comment.
Could you rename this to toggleDefaultFlow or similar?
Or at least extract source.default && source.default.id === flow.id into a labeled expression that communicate the toggling intent :)
There was a problem hiding this comment.
Renamed to "toggleDefaultFlow"
| mixins: [highlightConfig, portsConfig, hideLabelOnDrag], | ||
| created() { | ||
| const flow = this.node.definition.default || null; | ||
| delete this.node.definition.default; |
There was a problem hiding this comment.
I'm a little confused why you're deleting and re-adding it - could you give some overview why this needs to be done this way?
There was a problem hiding this comment.
Is to fix a weird issue when loading a process with a gateway with a defined "default" property, it is not reactive, then the user can not change the default flow. This could caused by way moddle loads this reference property
| get() { | ||
| return this.shape.attr('line').sourceMarker['stroke-width'] > 0; | ||
| }, |
There was a problem hiding this comment.
This getter doesn't seem to be called anywhere - and I think that we may be confused in future about whether we should use the defaultFlow computed property or the isDefaultFlow() method to determine whether a flow is the default. Could this be removed?
| set(value) { | ||
| this.shape.attr('line', { | ||
| sourceMarker: { | ||
| 'stroke-width': value ? 2 : 0, | ||
| }, | ||
| }); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
If you get rid of the getter then this can be its own method, removing the need for the computed property.
There was a problem hiding this comment.
Computed removed, changed to a setDefaultMarker method
| if (newNameLabel !== this.nameLabel) { | ||
| this.nameLabel = newNameLabel; | ||
| } | ||
| this.defaultFlow = this.isDefaultFlow(); |
There was a problem hiding this comment.
See comments above regarding the computed property, this could be a call to whatever method handles the stroke at the flow origin
| const isDefault = this.isDefaultFlow(); | ||
| this.shape.attr('line', { | ||
| sourceMarker: { | ||
| 'type': 'polyline', | ||
| 'stroke-width': isDefault ? 2 : 0, | ||
| points: '2,6 6,-6', | ||
| }, |
There was a problem hiding this comment.
This could be at the heart of the method that you have instead of the computed property. I'd also suggest inlining the isDefault const.
src/store.js
Outdated
| setDefaultFlow(state, { source, flow }) { | ||
| source.set('default', flow); | ||
| makeDefinitionPropertyReactive(source, 'default', flow); | ||
| }, |
There was a problem hiding this comment.
This doesn't need to be on the store - since it doesn't read or set any state in the store. You can basically inline the source.set in Modeler.vue where you commit (i.e. store.commit('setDefaultFlow', { source, flow });)
There was a problem hiding this comment.
Removed from store, moved to Modeler.vue
|
@johnblackspear thanks for your comments, I updated the PR with the fixes |
4 participants
Resolves #1182
This PR includes: