Apply condition sets to drawing objects in a layout + Static styling for objects. #2767

shefalijoshi · 2020-03-23T18:53:34Z

Static styles for drawing objects Resolves #2762
Conditional styles for drawing objects
Static Styles for domain objects
Control visibility for domain objects and drawing objects in a layout Resolves #2766
Add condition sets (one or more) to inspector view Resolves #2644
Default style for domain objects Resolves #2643

Author Checklist

Changes address original issue? Yes
Unit tests included and/or updated with changes? Yes
Command line build passes? Yes
Changes have been smoke-tested? Yes

…tor domain objects only. Needs Architectural review

…into conditional-styles

… into conditional-styles

…into conditional-styles

…m/nasa/openmct into conditional-styles

Removes hardcoded conditionSet work used for prototype

…into conditional-styles

… to the domain object.

Change variable name for better readability and clarity Remove unused code

…omainObjects

…ve performance

…into conditional-styles

Pass missing arguments

- Tweaks for layout/overflow issues; - Better header alignment; - Much better approach to "accordion" spacing;

…into conditional-styles

- New `.c-selector` class; - Markup added to tree-item.vue to fix missing `c-object-label` wrapper;

JoelMcKinnon

looks good

- Added new $glyph-icon-eye-disabled font symbol and related class; - Modded glyphs used for visibility toggle button; - Modded StyleEditor thumb preview to reflect visibility setting; - Tweaked `is-style-invisible` opacity property; - Code cleanup;

- Display disabled toolbar for Inspector styles in Browse mode; - Better approach to 'is-style-invisible' `c-style-thumb` with new bgCheckerboard mixin; - Moved `$controlDisabledOpacity` into a theme constant; - Refined spacing in Inspector grid;

akhenry · 2020-03-24T14:38:29Z

src/plugins/condition/StyleRuleManager.js

+        } else {
+            if (this.currentStyle) {
+                Object.keys(this.currentStyle).forEach(key => {
+                    this.currentStyle[key] = 'transparent';


Looks like this is used later on to identify which styles should be deleted from the element's style definition. If so, can we use something other than transparent? This is a valid CSS value for some style rules, so it may not be clear to developers that this will result in the subsequent deletion of this style rule. Could use a distinct value save in a named constant instead?

Yes. Will do.

akhenry · 2020-03-24T14:54:02Z

src/plugins/condition/components/ConditionDescription.vue

+
+<script>
+import {TRIGGER} from "@/plugins/condition/utils/constants";
+import { OPERATIONS } from "@/plugins/condition/utils/operations";


Inconsistent spacing here

akhenry · 2020-03-24T15:12:31Z

src/plugins/condition/components/ConditionDescription.vue

+                });
+                if (this.condition.isDefault) {
+                    this.criterionDescriptions.splice(0, 0, {
+                        description: 'all else fails'


Why is criterionDescriptions an array of objects rather than an array of Strings?

Ah! initially i was using the same component to show errors but then I moved errors into its own component. I've changed this to be strings.

akhenry · 2020-03-24T15:21:27Z

src/plugins/condition/components/ConditionDescription.vue

+            });
+        },
+        getOperatorText(operationName, values) {
+            const found = OPERATIONS.find((operation) => operation.name === operationName);


OPERATIONS should be an object keyed on the value of name to avoid an O(n) array scan.

This change will affect a lot of things. I'll create an issue for it and fix later.
#2771

akhenry · 2020-03-24T15:23:41Z

src/plugins/condition/components/ConditionError.vue

+    data() {
+        return {
+            conditionErrors: [],
+            ERROR: {


Doesn't need to be returned by data. Vue will make it reactive unnecessarily. Let's just make it a const.

akhenry · 2020-03-24T15:26:31Z

src/plugins/condition/components/ConditionError.vue

+            }
+        },
+        getCriterionErrors(criterion, index) {
+            this.openmct.objects.get(criterion.telemetry).then((telemetryObject) => {


There's way too much getting of objects. This is already an issue that users complain about. Criterion objects should evaluate a description and an error message only once - when the telemetry property changes and then store them.

akhenry · 2020-03-24T16:17:35Z

src/plugins/condition/components/inspector/ConditionSetDialogTreeItem.vue

+        }
+    },
+    mounted() {
+        // TODO: should update on mutation.


I don't think any of these todo items are relevant in this context?

akhenry · 2020-03-24T16:36:09Z

@shefalijoshi Won't be able to get through this today, code diff is huge. So that testing is not blocked, could you please just fix the code style issues reported by Circle CI, then I'll merge and file a followup with any changes necessary.

akhenry · 2020-03-24T18:02:55Z

src/plugins/condition/components/inspector/ConditionalStylesView.vue

    },
+    // mixins: [ObjectLink],


Let's remove this commented code.

src/plugins/condition/StyleRuleManager.js

davetsay · 2020-03-24T18:28:18Z

src/plugins/condition/components/Condition.vue

-                </span>
-            </template>
+            <span v-else>
+                <condition-description :show-label="false"


good choice to make as its own component. we just need to be sure the styling remains the same. the description component is inline in read-only mode and as a block element in edit mode.

@charlesh88 Can you take a look at the CSS here?

And I'm not trying to say its not working. Just making sure this was verified. The markup was slightly different. One block used divs and one used spans. Maybe the container elements take care of the proper displays.

davetsay · 2020-03-24T18:38:16Z

src/ui/toolbar/components/toolbar-button.vue

@@ -36,7 +36,7 @@ export default {
    },
    methods: {
        onClick(event) {
-            if (this.options.dialog) {
+            if ((this.options.isEditing === undefined || this.options.isEditing) && this.options.dialog) {


i see (this.options.isEditing === undefined) || this.options.isEditing) several times. i know it'd be extra code but i wonder if this should be in a computed with a descriptive name so that someone doesn't come along and mistakenly stomp over this thinking it can be a simple truthy check.

OK. Why a computed property? Could it be a regular method? like

showDialog() { return (this.options.isEditing === undefined || this.options.isEditing); }

Method would work fine if used for pure js logic. The computed would allow vue to cache the result if this is to be used in templates.

davetsay · 2020-03-24T18:42:35Z

src/plugins/condition/components/ConditionError.vue

+    <div v-for="(error, index) in conditionErrors"
+         :key="index"
+         class="u-alert u-alert--block u-alert--with-icon"
+    >{{ error.message.errorText }} <template v-if="error.additionalInfo">{{ error.additionalInfo }}</template>


do you need the v-if? if error.additionalInfo is undefined then nothing will be rendered

…into conditionals-use-conditionSet

davetsay

I don't have anything major to report. Just suggestions. Andrews comments are much more critical to address.

davetsay · 2020-03-24T18:55:15Z

src/plugins/condition/components/inspector/ConditionSetDialogTreeItem.vue

+    },
+    data() {
+        return {
+            hasChildren: false,


can we have this as a computed based on children.length or is this a way to get around an async call for children?

Yes. To avoid additional calls.

davetsay · 2020-03-24T19:43:01Z

src/plugins/condition/components/inspector/ConditionSetDialogTreeItem.vue

+    computed: {
+        navigated() {
+            const itemId = this.selectedItem && this.selectedItem.itemId;
+            const isSelectedObject = itemId && this.openmct.objects.areIdsEqual(this.node.object.identifier, itemId);


i think areIdsEqual would not be true if itemId is undefined so I think you are safe removing itemId check

True. I can check how it behaves without that.

davetsay · 2020-03-24T19:45:16Z

src/plugins/condition/components/inspector/ConditionSetDialogTreeItem.vue

+        removeChild(identifier) {
+            let removeId = this.openmct.objects.makeKeyString(identifier);
+            this.children = this.children
+                .filter(c => c.id !== removeId);


uncharacteristic non-descriptive var name for you @shefalijoshi , 😉

davetsay · 2020-03-24T20:05:46Z

src/plugins/condition/components/inspector/StyleEditor.vue

+            } else {
+                this.styleItem.style[item.property] = value;
+            }
+            this.$emit('persist', this.styleItem);


maybe rename to reflect it has updated style. whatever is listening to this is going to be persisting i think.

Sure. I can rename the event to persistUpdatedStyle

src/plugins/displayLayout/mixins/objectlStyles-mixin.js

src/plugins/telemetryTable/components/table-configuration.vue

davetsay · 2020-03-24T20:53:23Z

src/plugins/condition/components/inspector/ConditionalStylesView.vue

+            this.openmct.objects.getOriginalPath(this.conditionSetDomainObject.identifier).then(
+                (objectPath) => {
+                    this.objectPath = objectPath;
+                    this.navigateToPath = '#/browse/' + this.objectPath


should this use a call to get root path instead of hardcoding?

Actually I think I can get rid of that property altogether.

akhenry · 2020-03-24T20:59:51Z

Merged to unblock testing. Will continue review and file followups.

shefalijoshi and others added 30 commits February 25, 2020 22:12

Hardcoded prototype - conditional styles for display layout or genera…

c1551fc

…tor domain objects only. Needs Architectural review

Updates to ConditionalStylesView

865009b

Adds background colors list

b9acaf4

Merge branch 'topic-conditionals' of https://github.com/nasa/openmct …

e961e81

…into conditional-styles

Merge branch 'topic-conditionals' of https://github.com/nasa/openmct …

6218c26

…into conditional-styles

Merge branch 'conditionals-refactor' of https://github.com/nasa/openmct…

1f04b1c

… into conditional-styles

Show conditional styles for subObjectViews and telemetryViews

99e2100

Merge branch 'topic-conditionals' of https://github.com/nasa/openmct …

3d6228a

…into conditional-styles

Merge branch 'topic-conditionals' of https://github.com/nasa/openmct …

5649978

…into conditional-styles

Merge branch 'dave/provide-conditions-telemetry' of https://github.co…

27f1015

…m/nasa/openmct into conditional-styles

Uses telemetry provider to determine which style is active

0c24868

Removes hardcoded conditionSet work used for prototype

Fixes failing test

b82633e

Merge branch 'topic-conditionals' of https://github.com/nasa/openmct …

a04f9fd

…into conditional-styles

Add default styles to conditionalStyles when a condition set is added…

cd524b8

… to the domain object.

Use EventEmitter alias instead of eventEmitter3 in imports

ddc48fa

Change variable name for better readability and clarity Remove unused code

Change StyleRuleManager to accept conditionStyle objects instead of d…

303ada5

…omainObjects

Uses a map for conditional styles instead of a list in order to impro…

d381187

…ve performance

Merge branch 'topic-conditionals' of https://github.com/nasa/openmct …

20ae39c

…into conditional-styles

Use in-built api to check for identifier equality

b195dce

Pass missing arguments

Removes unnecessary object get call.

0b85e7e

Conditionals styling WIP

408e27c

- Tweaks for layout/overflow issues; - Better header alignment; - Much better approach to "accordion" spacing;

Adds conditional styles for drawing objects

8a2e406

Fixes small bugs while adding or removing nested conditional styling

788678f

Removes hard coded conditionSetIdentifier

a4e01da

Merge branch 'topic-conditionals' of https://github.com/nasa/openmct …

9c60cc1

…into conditional-styles

Fixes small conditionManager bug

7ff331d

WIP - Adds condition set selection dialog

77097fb

WIP - Add condition set

4482759

Styling for tree dialog in selector

1d9fa2e

- New `.c-selector` class; - Markup added to tree-item.vue to fix missing `c-object-label` wrapper;

Merge remote-tracking branch 'origin' into conditionals-styling-2

f6d2f9f

JoelMcKinnon approved these changes Mar 23, 2020

View reviewed changes

charlesh88 added 2 commits March 23, 2020 18:22

Conditionals styling WIP

ec5af55

- Added new $glyph-icon-eye-disabled font symbol and related class; - Modded glyphs used for visibility toggle button; - Modded StyleEditor thumb preview to reflect visibility setting; - Tweaked `is-style-invisible` opacity property; - Code cleanup;

Conditionals styling WIP

278c988

- Display disabled toolbar for Inspector styles in Browse mode; - Better approach to 'is-style-invisible' `c-style-thumb` with new bgCheckerboard mixin; - Moved `$controlDisabledOpacity` into a theme constant; - Refined spacing in Inspector grid;

akhenry reviewed Mar 24, 2020

View reviewed changes

davetsay added the Target: VIPER label Mar 24, 2020

akhenry reviewed Mar 24, 2020

View reviewed changes

shefalijoshi added 2 commits March 24, 2020 11:12

Fixes linting issues

5fe4ed7

Fixes telemetry metadata name

4e1b3aa

davetsay reviewed Mar 24, 2020

View reviewed changes

src/plugins/condition/StyleRuleManager.js Show resolved Hide resolved

shefalijoshi added 2 commits March 24, 2020 11:26

criterion descriptions is now an array of strings

1e4c817

Fixes spacing

57e3134

davetsay reviewed Mar 24, 2020

View reviewed changes

Removes commented out code.

070bc6d

davetsay reviewed Mar 24, 2020

View reviewed changes

Merge branch 'topic-conditionals' of https://github.com/nasa/openmct …

7e3c748

…into conditionals-use-conditionSet

shefalijoshi requested review from akhenry and davetsay March 24, 2020 18:49

Remove v-if

c5e17b4

akhenry merged commit 57a68a2 into topic-conditionals Mar 24, 2020

davetsay reviewed Mar 24, 2020

View reviewed changes

akhenry mentioned this pull request Mar 28, 2020

Conditionals refactoring #2817

Closed

13 tasks

Apply condition sets to drawing objects in a layout + Static styling for objects. #2767

Apply condition sets to drawing objects in a layout + Static styling for objects. #2767

Conversation

shefalijoshi commented Mar 23, 2020 • edited Loading

JoelMcKinnon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhenry Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhenry Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shefalijoshi Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

akhenry commented Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shefalijoshi Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shefalijoshi Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

davetsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhenry commented Mar 24, 2020

shefalijoshi commented Mar 23, 2020 •

edited

Loading

akhenry Mar 24, 2020 •

edited

Loading

akhenry Mar 24, 2020 •

edited

Loading

shefalijoshi Mar 24, 2020 •

edited

Loading

akhenry commented Mar 24, 2020 •

edited

Loading

shefalijoshi Mar 24, 2020 •

edited

Loading

shefalijoshi Mar 24, 2020 •

edited

Loading