-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix(cloud-templates): cast default number and boolean numbers to feel #121
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 2 additions & 1 deletion
3
src/cloud-element-templates/properties-panel/properties/custom-properties/NumberProperty.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
export const isSpecialFeelProperty = (property) => { | ||
return [ 'optional', 'static' ].includes(property.feel) && [ 'Boolean', 'Number' ].includes(property.type); | ||
}; | ||
|
||
export const toFeelExpression = (value, type) => { | ||
if (typeof value === 'string' && value.startsWith('=')) { | ||
return value; | ||
} | ||
|
||
if (type === 'Boolean') { | ||
value = value === 'false' ? false : value; | ||
return '=' + !!value; | ||
} | ||
|
||
if (typeof value === 'undefined') { | ||
return value; | ||
} | ||
|
||
return '=' + value.toString(); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" id="Definitions_039bbyr" targetNamespace="http://bpmn.io/schema/bpmn" exporter="Camunda Modeler" exporterVersion="4.4.0"> | ||
<bpmn:process id="Process_1" isExecutable="true"> | ||
<bpmn:serviceTask id="Task_1" /> | ||
<bpmn:intermediateThrowEvent id="Event_1" /> | ||
</bpmn:process> | ||
<bpmndi:BPMNDiagram id="BPMNDiagram_1"> | ||
<bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_1"> | ||
<bpmndi:BPMNShape id="Task_1_di" bpmnElement="Task_1"> | ||
<dc:Bounds x="0" y="0" width="100" height="80" /> | ||
</bpmndi:BPMNShape> | ||
<bpmndi:BPMNShape id="Event_0f371po_di" bpmnElement="Event_1"> | ||
<dc:Bounds x="92" y="172" width="36" height="36" /> | ||
</bpmndi:BPMNShape> | ||
</bpmndi:BPMNPlane> | ||
</bpmndi:BPMNDiagram> | ||
</bpmn:definitions> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
[ | ||
{ | ||
"$schema": "https://unpkg.com/@camunda/zeebe-element-templates-json-schema/resources/schema.json", | ||
"name": "booleanField optional", | ||
"id": "booleanField.feel.optional", | ||
"appliesTo": [ | ||
"bpmn:ServiceTask" | ||
], | ||
"properties": [ | ||
{ | ||
"label": "DefaultStaticBooleanProperty", | ||
"type": "Boolean", | ||
"feel": "static", | ||
"value": true, | ||
"binding": { | ||
"type": "zeebe:property", | ||
"name": "StaticBooleanProperty" | ||
} | ||
}, | ||
{ | ||
"label": "DefaultOptionalBooleanProperty", | ||
"type": "Boolean", | ||
"feel": "optional", | ||
"value": true, | ||
"binding": { | ||
"type": "zeebe:property", | ||
"name": "OptionalBooleanProperty" | ||
} | ||
} | ||
] | ||
}, | ||
{ | ||
"$schema": "https://unpkg.com/@camunda/zeebe-element-templates-json-schema/resources/schema.json", | ||
"name": "FEEL optional", | ||
"id": "numberField.feel.optional", | ||
"appliesTo": [ | ||
"bpmn:ServiceTask" | ||
], | ||
"properties": [ | ||
{ | ||
"label": "DefaultStaticNumberProperty", | ||
"type": "Number", | ||
"feel": "static", | ||
"value": 123, | ||
"binding": { | ||
"type": "zeebe:property", | ||
"name": "StaticNumberProperty" | ||
} | ||
}, | ||
{ | ||
"label": "DefaultOptionalNumberProperty", | ||
"type": "Number", | ||
"feel": "optional", | ||
"value": 123, | ||
"binding": { | ||
"type": "zeebe:property", | ||
"name": "OptionalNumberProperty" | ||
} | ||
} | ||
] | ||
} | ||
] |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit-pick: Let's rename the utility to
CamelCase
, i.e.FeelUtil
.I acknowledge that we already have some
lowerCaseYesThatIsMe
files in the code-base, but let's not introduce too many more of them, as the standard is starting with a capital letter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that the convention we have? As I see it, the file name reflects the default export. Per convention, Classes and Components are written in PascalCase, hence a lot of our files are also PascalCase.
The utility files do not have a default export or exports a function. Since a function is usually camelCase (ignoring functional components), the file name will reflect this as well.
This is consistent with all existing utility files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cf. https://github.com/bpmn-io/bpmn-js-element-templates/blob/e8d563a5b4b5abcdaf88ffada30fd9c0c914c95e/src/cloud-element-templates/util/validate.js as an example.
Validator
is a class, since it is inValidator.js
. When making it a utilityvalidate
, the file name will reflect the contents and is namedvalidate.js
(lower case)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can only look towards our different utilities in the eco-system, and they all seem to be historically upper-case - https://github.com/bpmn-io/diagram-js/tree/develop/lib/util.
Only recently we started lower-casing things in side-projects, but also not consistently. Predominently, utilities are still upper case, unless imported via
index.js
directly - https://github.com/bpmn-io/bpmn-js-element-templates/tree/main/src/utils.👍 on reflecting the name / type of the default export, where it makes sense. But your utility does not provide a default export, but is a collection of various things - https://github.com/bpmn-io/bpmn-js-element-templates/pull/121/files#diff-ed2c638650f4cb053378f288c21bb35939630f122e9d0eb79f4924d62984a084.
Another debate is if "single function per file", with a default context is an appropriate way to slice things. Sometimes (but only sometimes) it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we are not consistent in our naming schemes. In the context of this JSX project, I would argue that using camelCase for utilities helps with understanding the file structure better. Using Upercase primarily for Components helps with knowing what the contents of the file will be without having to open the file itself.
Only if they are closely related to a bpmn-js utility they are extending:
https://github.com/bpmn-io/bpmn-js-element-templates/tree/main/src/cloud-element-templates/util
While the utility file does not have a default export, it still only exports functions and should therefore follow the naming convention for functions, rather than Components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No hard opinion, but if we want to keep naming consistent, I'd follow the unofficial core libraries rule of PascalCase everywhere (cf. bpmn-js and diagram-js).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted to PascalCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, stuff is usually auto-imported. For that reason, I'd discourage default exports.