Summary Widgets #1668

adoubekk · 2017-08-08T19:18:41Z

References issues #1644, #1654, and #1669.
Uses new view API implementation from #1642.

Initial pull request for Summary Widgets for Open MCT.

@charlesh88 @larkin Any preliminary feedback appreciated.

Author Checklist

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

Update view provider to allow metadata definitions and to play nicely with old style views. Spec out some updates to ViewProviders and ViewRegistry to support further use of views.

Add a summary widget domain object type Implement basic interface and style configuration for rules

Add rule configuration inputs, populated with domain objects, metadata, and appropriate operations for a given type

Fixes #1654 Updated font files and icomoon json file;

Fixes #1654

Fixes #1654 Markup cleanups

Add delete functionality and UI Improve process for indexing and rendering rules to support deletion, and eventually reordering Fix bug where default style was passed by reference and overwritten on style updates

Fixes #1654 Ported .inspector-config SCSS from _inspector.scss and renamed to .l-compact-form;

Fixes #1654 New SCSS for widgets; markup mods in progress

Fixes #1654 Ported .view-control SCSS out of tree context to allow more independent use;

Fixes #1654 Significant mods to markup and SCSS;

Fixes #1654 Minor tweaks; class renaming; restructured main containers in ruleTemplate.html; minor updates for class names in WidgetView.js;

Fixes #1654 Merge latest from summary-widgets branch

Fixes #1654 Fixes and tweaks post-merge

Fixes #1654 Add condition duplicate and delete buttons

Merge remote-tracking branch 'origin/summary-widgets-styling' into summary-widgets Apply new style and wire up new markup for interaction

…dgets-styling

Fixes #1654

Fixes #1654 Mod .grippy to point at new glyph class; Stubbed in icon palette control in markup;

Add additional implemenation for new markup. Rules now store label and message, and label is applied to widget. Implementation for duplicate.

Issue #1644 Merge and resolve conflicts with style branch Fix rule indexing bug

Fixes #1654 Refactor .l-color-palette to .l-palette, includes file renaming; add icon-palette in WidgetView.js

Issue #1644 Fix merge issue related to inputs for label and message

Issue #1644 Wire up icon palette inputs to widget, and make icon class a persistable property of a rule

Issue #1644 Add implementation and persistability for a single condition configuration field. Baseline for adding arbitrary numbers of rules

Support configuring and persisting multiple conditions per rule

Re-implement widgets in with a module-oriented structure. Fix issues related to asychronous loading of telemetry and composition. Package inputs as re-usable, Zepto-based modules.

Dynamically add value inputs when an operation is being configured. Cleanup code related to removed label caching feature

… rules left, includes some code cleanup

[Summary Widget] Added margin to the delete button (#1762)

…ch for changes unsubscribe on destroy

akhenry · 2017-11-07T19:59:57Z

@larkin @VWoeltjen This is implemented using no framework other than Zepto. That's fine, but is this going to be our approach going forward? I think we need to settle on a framework (where 'no framework' is a valid option) before we merge any major new pieces of functionality.

akhenry · 2017-11-09T01:12:41Z

Testing note: There's a lot going on in Summary Widgets, and a lot of it uses observers. It's all fairly self-contained, but still I'd like to make sure that our testing looks out for memory leaks here.

akhenry

The quality of this code is excellent, and clearly represents a lot of hard work. Well done everyone involved! Apologies if my brief comments sometimes seem curt, I was just trying to keep them succinct, I think the code quality is really high here.

The review contains some issues that I think we should address before merging into master. For other general refactoring issues I've created followup issue #1804

I've also created a couple of other issues for later refactoring once other pieces of API are in place, namely #1801 and #1802. I've also created issue #1803 to address refactoring once we have established post-Angular 1 approach to internal view development.

akhenry · 2017-10-25T18:41:42Z

src/plugins/summaryWidget/SummaryWidgetsCompositionPolicy.js

-            var domainObject = legacyObject.useCapability('adapter');
-            var providers = openmct.mainViews.get(domainObject);
-            $scope.view = providers[0] && providers[0].view(domainObject);
+        /**


Comment is out of date. A comment is arguably not necessary here at all, it's a small class and the code is pretty self explanatory.

akhenry · 2017-10-26T20:43:50Z

src/plugins/summaryWidget/plugin.js

+            return {
+                name: 'Widget View',
+                view: function (domainObject) {
+                    var summaryWidget = new SummaryWidget(domainObject, openmct);


The SummaryWidget conforms to the View interface, we could just return it rather than wrapping it in another object.

akhenry · 2017-10-26T20:44:36Z

src/plugins/summaryWidget/plugin.js

+        return function install(openmct) {
+            openmct.types.addType('summary-widget', widgetType);
+            openmct.objectViews.addProvider(initViewProvider(openmct));
+            openmct.legacyExtension('policies', {category: 'composition',


Formatting of the policy definition could be improved

akhenry · 2017-10-26T20:55:10Z

src/plugins/summaryWidget/res/ruleTemplate.html

+                            <select class="t-trigger">
+                                <option value="any">any condition is met</option>
+                                <option value="all">all conditions are met</option>
+                                <!-- <option value="js">the following JavaScript evaluates to true</option> -->


Remove commented code

akhenry · 2017-10-26T20:55:27Z

src/plugins/summaryWidget/res/ruleTemplate.html

+                         </div>
+                    </span>
+                </li>
+                <!-- <li class="t-rule-js-condition-input-holder">


Remove commented code

akhenry · 2017-11-08T23:45:47Z

src/plugins/summaryWidget/src/ConditionManager.js

+            self = this;
+
+        self.telemetryTypesById[object.identifier.key] = {};
+        return telemetryAPI.request(object, {}).then(function (telemetry) {


Type inference from historical data is not the right approach here, but it's a failing of the API. I've created a separate issue to address this later - #1801

Because this is also populating the subscription cache at the same time I can live with it for now, but at the very least, it should infer type from the value returned by the telemetry formatter's .parse() function, and not from the telemetry datum directly.

akhenry · 2017-11-08T23:58:46Z

src/plugins/summaryWidget/src/ConditionManager.js

+            self = this;
+
+        self.telemetryTypesById[object.identifier.key] = {};
+        return telemetryAPI.request(object, {}).then(function (telemetry) {


Although it's not documented, the request propertiessize: 1 and strategy: 'latest' can be added to historical requests to restrict the amount of data that is brought back to just the most recent datum.

@larkin Since this is quite useful we should document this and make it official (though dependent upon the data store supporting it).

I could not get this to work, maybe leave this change for v1.1?

They're optional parameters, not all backends will support them (SWG does not, from memory). The request parameters should at least be set though, so that backends that do support them can minimize the amount of data sent back.

This should be documented. I am pulling out a set of changes from the plot that adds this behavior to all example providers.

akhenry · 2017-11-09T00:13:25Z

src/plugins/summaryWidget/src/SummaryWidget.js

+            ruleOrder = this.domainObject.configuration.ruleOrder;
+
+        while (Object.keys(this.rulesById).includes('rule' + ruleCount)) {
+            ruleCount = ++ruleCount;


Redundant assignment

akhenry · 2017-11-09T01:54:35Z

src/plugins/summaryWidget/src/SummaryWidget.js

        this.domainObject.configuration.ruleOrder = ruleOrder;
-        this.updateDomainObject();
+
+        if(this.editing){


We can remove this once the root cause is addressed (see other review comments)

akhenry · 2017-11-09T01:55:06Z

src/plugins/summaryWidget/src/SummaryWidget.js

     * Mutate this domain object's configuration with the current local configuration
     */
    SummaryWidget.prototype.updateDomainObject = function () {
+        console.log('persisting');


Remove debugging code.

…rsist calls in Rule.js

…go down from 3 to 2. Prevent HTML injection in summary widget labels. Change default name of summary widget to 'Unnamed Rule' to avoid confusion.

…tch for changes unsubscribe on destroy Remove unnecessary persist calls in Rule.js.

deeptailor · 2017-11-24T20:41:56Z

@akhenry, @larkin - I included all the most recent changes from master. Please review.

…refreshConditions and add it after refreshConditions to initCondition and deleteCondition

…rn summary widget instance in plugin.js instead of wrapping in new object for view

…t it

no longer main reviewer

akhenry · 2017-11-28T21:02:29Z

Reviewer Checklist

Changes appear to address issue? Y
Appropriate unit tests included? Y
Code style and in-line documentation are appropriate? Y
Commit messages meet standards? Y

Pete Richards and others added 30 commits July 5, 2017 13:56

[ViewAPI] Update view API with more support

ba98d93

Update view provider to allow metadata definitions and to play nicely with old style views. Spec out some updates to ViewProviders and ViewRegistry to support further use of views.

[Summary Widgets] Add summary widgets, WIP

54980fb

Add a summary widget domain object type Implement basic interface and style configuration for rules

[Summary Widgets] Populate rule config inputs

cfbff02

Add rule configuration inputs, populated with domain objects, metadata, and appropriate operations for a given type

[Frontend] Add new summary-widget glyph

55e2a77

Fixes #1654 Updated font files and icomoon json file;

[Frontend] Apply new summary-widget glyph

4c9235b

Fixes #1654

[Frontend] WIP summary widget styling

c73f725

Fixes #1654 Markup cleanups

[Summary Widgets] Make Rules Deletable

e68e0c3

Add delete functionality and UI Improve process for indexing and rendering rules to support deletion, and eventually reordering Fix bug where default style was passed by reference and overwritten on style updates

[Frontend] WIP summary widget styling

bef7940

Fixes #1654 Ported .inspector-config SCSS from _inspector.scss and renamed to .l-compact-form;

[Frontend] WIP summary widget styling

597327f

Fixes #1654 New SCSS for widgets; markup mods in progress

[Frontend] WIP summary widget styling

826ac3a

Fixes #1654 Ported .view-control SCSS out of tree context to allow more independent use;

[Frontend] WIP summary widget styling

a103b4d

Fixes #1654 Significant mods to markup and SCSS;

[Frontend] WIP summary widget styling

8466723

Fixes #1654 Minor tweaks; class renaming; restructured main containers in ruleTemplate.html; minor updates for class names in WidgetView.js;

[Frontend] WIP summary widget styling

b55f43b

Fixes #1654 Merge latest from summary-widgets branch

[Summary Widgets] Minor UI implementation

c83de8a

[Frontend] WIP summary widget styling

827a283

Fixes #1654 Fixes and tweaks post-merge

[Frontend] WIP summary widget styling

8aeb365

Fixes #1654 Add condition duplicate and delete buttons

[Style] Merge with style branch

c051f34

Merge remote-tracking branch 'origin/summary-widgets-styling' into summary-widgets Apply new style and wire up new markup for interaction

Merge remote-tracking branch 'origin/summary-widgets' into summary-wi…

2aa04b0

…dgets-styling

[Frontend] Add new icon-grippy

500733a

Fixes #1654

[Frontend] WIP summary widget styling

d15446a

Fixes #1654 Mod .grippy to point at new glyph class; Stubbed in icon palette control in markup;

[Summary Widgets] Link markup to implementation

eab702b

Add additional implemenation for new markup. Rules now store label and message, and label is applied to widget. Implementation for duplicate.

[Summary Widgets] Merge with style

755c013

Issue #1644 Merge and resolve conflicts with style branch Fix rule indexing bug

[Frontend] WIP summary widget styling

e1e2dca

Fixes #1654 Refactor .l-color-palette to .l-palette, includes file renaming; add icon-palette in WidgetView.js

[Bug Fix] Fix text input handlers

0fa8472

Issue #1644 Fix merge issue related to inputs for label and message

[Inputs] Add implementation for icon palette

4b3be4c

Issue #1644 Wire up icon palette inputs to widget, and make icon class a persistable property of a rule

[Summary Widgets] Make conditions persistable

a1596d0

Issue #1644 Add implementation and persistability for a single condition configuration field. Baseline for adding arbitrary numbers of rules

[Summary Widgets] Implementation for conditions

98a9d71

Support configuring and persisting multiple conditions per rule

[Refactor] Overhaul of code structure

4a35508

Re-implement widgets in with a module-oriented structure. Fix issues related to asychronous loading of telemetry and composition. Package inputs as re-usable, Zepto-based modules.

[Refactor] Cleanup old files

405bb55

[Inputs] Add value inputs for rule configuration

eacc181

Dynamically add value inputs when an operation is being configured. Cleanup code related to removed label caching feature

deeptailor and others added 4 commits October 10, 2017 10:33

fix bug regarding grippy persisting on rule0 even if there are only 2…

8bc3621

… rules left, includes some code cleanup

change default label to 'Unnamed Rule' to avoid confusion

e6beaf2

Merge pull request #1768 from savva-k/button-distance-1762

e7e4c2f

[Summary Widget] Added margin to the delete button (#1762)

prevent persist on initialize by checkingif in edit mode, and add wat…

5c93798

…ch for changes unsubscribe on destroy

akhenry requested changes Nov 9, 2017

View reviewed changes

akhenry assigned deeptailor Nov 9, 2017

deeptailor and others added 10 commits November 15, 2017 11:07

changes requested by reviewer + find and fix causes of unnecessary pe…

82a35cb

…rsist calls in Rule.js

Fix grippy bug, where grippies arent correctly showing up when rules …

b9a678b

…go down from 3 to 2. Prevent HTML injection in summary widget labels. Change default name of summary widget to 'Unnamed Rule' to avoid confusion.

[Summary Widget] Added margin to the delete button (#1762)

a4f2033

prevent persist on initialize by checking if in edit mode, and add wa…

28da6a5

…tch for changes unsubscribe on destroy Remove unnecessary persist calls in Rule.js.

squash commits

7ee4a50

pull from master

c40cc3e

merge with master

4ec6f79

add key property to view provider to comply with new View API

c931c54

update with master

0bc05e1

fix plugin.js file to include summary widgets

00f4dcd

deeptailor force-pushed the summary-widgets branch from 82a35cb to 00f4dcd Compare November 24, 2017 20:35

deeptailor added 6 commits November 26, 2017 09:01

merge from latest summary widgets

1f4a3f4

make changes requested by reviewer - remove generateDescription from …

9efc525

…refreshConditions and add it after refreshConditions to initCondition and deleteCondition

fix checkstyle error

b18c09f

Throw error when unsupported callback is passed in condition.js, retu…

81d349b

…rn summary widget instance in plugin.js instead of wrapping in new object for view

add request properties to telemetry request for providers that suppor…

c863ab4

…t it

remove check for editing when persisting, in SummaryWidget.js

4fb0dac

akhenry approved these changes Nov 28, 2017

View reviewed changes

akhenry merged commit 4b07930 into master Nov 28, 2017

Summary Widgets #1668

Summary Widgets #1668

Uh oh!

Conversation

adoubekk commented Aug 8, 2017 • edited by deeptailor Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Author Checklist

Uh oh!

akhenry commented Nov 7, 2017

Uh oh!

akhenry commented Nov 9, 2017

Uh oh!

akhenry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhenry Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhenry Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deeptailor commented Nov 24, 2017

Uh oh!

akhenry commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

adoubekk commented Aug 8, 2017 •

edited by deeptailor

Loading

akhenry Nov 8, 2017 •

edited

Loading

akhenry Nov 8, 2017 •

edited

Loading

akhenry commented Nov 28, 2017 •

edited

Loading