Conditionals feature #2830
Conditionals feature #2830
Conversation
Condition clone name persistance
Removes console log statements Fixes import paths
Clone condition, criteria issues
…2799) * Cast number inputs to a Number so that operations evaluate correctly
* New Condition Widget * Add new glyphs and symbols font files
* Adds XOR and NOT triggers for conditions * Adds unit tests and fixes linting issues
* Test Data * Persist testData and apply it to the conditionSet * Do not persist the applied flag, but persist test data Co-authored-by: charlesh88 <[email protected]>
Removed image properties button from display layouts toolbar
Any all telemetry option for conditions.
* Sisplay default condition output by... default
Topic conditionals master merge
| @@ -0,0 +1,81 @@ | |||
| /***************************************************************************** | |||
- Restore updated symbols font file for Condition Widget icon glyph; - Change "All telemetry | Any telemetry" to all lowercase; - Fix cursor styles for Condition Widget; - Fix Safari layout problems with Condition Set/section styles;
… commented out code
There was a problem hiding this comment.
Ok, I've got through the JS files, will hit the Vue files next.
This is looking really great, you guys have put in a tonne of work here in a short amount of time (even if it didn't feel like a short amount of time 😄 ).
There are a lot of comments here, but they are mostly small niggles. Happy to discuss any or all of these.
| * ] | ||
| * } | ||
| */ | ||
| export default class ConditionClass extends EventEmitter { |
There was a problem hiding this comment.
Let's not have a class name suffixed with Class. Condition should be clear enough.
| } | ||
|
|
||
| handleBroadcastTelemetry(datum) { | ||
| if (!datum || !datum.id) { |
There was a problem hiding this comment.
Why would this ever happen?
| } | ||
| this.criteria.forEach(criterion => { | ||
| if (criterion.telemetry && (criterion.telemetry === 'all' || criterion.telemetry === 'any')) { | ||
| criterion.handleSubscription(datum, this.conditionManager.telemetryObjects); |
There was a problem hiding this comment.
Let's name this something like "evaluateTelemetry" or something. As mentioned previously, naming it "handleSubscription" makes it sound as though it is handling subscription lifecycle events.
There was a problem hiding this comment.
This has been addressed and merged in #2904
|
|
||
| updateTelemetry() { | ||
| this.criteria.forEach((criterion) => { | ||
| criterion.updateTelemetry(this.conditionManager.telemetryObjects); |
There was a problem hiding this comment.
This is very confusingly named. This function updates the telemetry object, not the telemetry itself.
| this.conditionManager.on('broadcastTelemetry', this.handleBroadcastTelemetry, this); | ||
| } | ||
|
|
||
| handleBroadcastTelemetry(datum) { |
There was a problem hiding this comment.
See comments I've made in the past about naming handlers. They should describe what they do, and not the event they handle. This should be something like processTelemetryDatum.
There was a problem hiding this comment.
This has been addressed and merged in #2904
| { | ||
| name: 'enumValueIsNot', | ||
| operation: function (input) { | ||
| return input[0] !== input[1]; |
There was a problem hiding this comment.
This was fixed and merged as part of #2898
| } | ||
| }, | ||
| { | ||
| name: 'valueIs', |
There was a problem hiding this comment.
I keep getting confused by the name of this operation, and the fault lies with the people who wrote the requirements, but this operation should be isOneOf.
| } | ||
| }, | ||
| { | ||
| name: 'valueIsNot', |
| operation: function (input) { | ||
| if (input[1]) { | ||
| const values = input[1].split(','); | ||
| return values.find((value) => input[0].toString() === _.trim(value.toString())); |
There was a problem hiding this comment.
This function should strictly return boolean values. Truthiness is not a substitute for truth. Array.some() might be better here as it returns a boolean.
| operation: function (input) { | ||
| if (input[1]) { | ||
| const values = input[1].split(','); | ||
| const found = values.find((value) => input[0].toString() === _.trim(value.toString())); |
There was a problem hiding this comment.
Let's use native functions wherever possible.
There was a problem hiding this comment.
Also, Array.some() might be better here as it returns a boolean (see previous comment about truthiness vs truth).
No description provided.