feat(#7394): Incorporate Status Indicators into the main Vue app #7395

ozyx · 2024-01-21T19:04:13Z

Describe your changes:

This is a bit of a refactor in order to support incorporating our Status Indicators into the main Vue app (AppLayout). This includes non-breaking changes to the IndicatorAPI which allow it to support directly rendering Vue components provided to the API either as Component objects themselves, or as HTMLElements (the original behavior, to be framework-agnostic). It introduces a new utility function vueWrapHtmlElement() that takes an HTMLElement and uses a render function to create an Anonymous Vue component to be dynamically rendered in the StatusIndicator bar.

This means:

We no longer create a new Vue app for each Indicator (slight perf boost, memory reduction)
All StatusIndicators are incorporated into the main Vue app (reactivity benefits)

This is an enhancement to support the VIPER S.A. work.

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Has this been smoke tested?
Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
Have you associated a milestone with this PR? Note: leave blank if unsure.
Is this a breaking change to be called out in the release notes?
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?

- Adds a new property to Indicators, `component`, which is a synchronous or asynchronous Vue component. - Adds `wrapHtmlElement` utility function to create anonymous Vue components out of `HTMLElement`s (for backwards compatibility) - Refactors StatusIndicators.vue to use dynamic components, allowing us to dynamically render indicators (and keep it all within Vue's ecosystem).

- Refactors some indicators to use Vue components directly as async components

codecov · 2024-01-21T19:08:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4cf6306) 55.88% compared to head (9a7eb7d) 55.98%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7395      +/-   ##
==========================================
+ Coverage   55.88%   55.98%   +0.09%     
==========================================
  Files         659      660       +1     
  Lines       26277    26282       +5     
  Branches     2549     2550       +1     
==========================================
+ Hits        14686    14715      +29     
+ Misses      10885    10857      -28     
- Partials      706      710       +4

Flag	Coverage Δ		*Carryforward flag
e2e-full	`41.03% <ø> (-0.05%)`	⬇️	Carriedforward from 7178135
e2e-stable	`60.14% <100.00%> (+0.07%)`	⬆️
unit	`48.93% <100.00%> (+0.10%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Files	Coverage Δ
src/api/indicators/IndicatorAPI.js	`100.00% <100.00%> (ø)`
...gins/clearData/components/GlobalClearIndicator.vue	`100.00% <ø> (ø)`
src/plugins/clearData/plugin.js	`100.00% <ø> (ø)`
src/plugins/clock/components/ClockIndicator.vue	`75.00% <ø> (+3.57%)`	⬆️
src/plugins/clock/plugin.js	`61.11% <100.00%> (-7.08%)`	⬇️
.../notebook/components/NotebookSnapshotIndicator.vue	`75.00% <100.00%> (+0.92%)`	⬆️
src/plugins/notebook/plugin.js	`97.29% <ø> (-0.14%)`	⬇️
...tionIndicator/components/NotificationIndicator.vue	`84.61% <ø> (+15.38%)`	⬆️
src/plugins/notificationIndicator/plugin.js	`100.00% <ø> (ø)`
src/ui/layout/AppLayout.vue	`20.40% <100.00%> (ø)`
... and 2 more

... and 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cf6306...9a7eb7d. Read the comment docs.

unlikelyzero

Great!

unlikelyzero · 2024-01-22T01:17:33Z

e2e/tests/functional/plugins/lad/ladSet.e2e.spec.js

@@ -53,6 +53,9 @@ test.describe('LAD Table Sets', () => {

    await page.goto(ladTableSet.url);

+    // Wait for the initial value to show after mount
+    await expect(page.getByLabel('lad value').first()).not.toContainText('---');


We will need to factor out the use of first at some point

I think it's ok if we're accessing a value in a table?

yeah, seems rational here

e2e/tests/visual-a11y/components/header.visual.spec.js

akhenry · 2024-01-23T02:14:59Z

Changes look good to me, thanks. Just needs some testing instructions. I tested this by verifying:

That the Notifications, Snapshots, Clear Data, and Clock indicators appear correctly, and still function as expected,
Verified that legacy HTML-based indicators still function as expected. I tested this by installing the performance indicator and verifying that it renders correctly when installed, and FPS values are updating and changing as expected.

The Performance Indicator can be installed like so:

openmct.install(openmct.plugins.PerformanceIndicator());

shefalijoshi · 2024-01-23T07:52:09Z

src/api/indicators/IndicatorAPI.js

   */
  add(indicator) {
    if (!indicator.priority) {
      indicator.priority = this.openmct.priority.DEFAULT;
    }
+    if (!indicator.vueComponent) {
+      indicator.vueComponent = vueWrapHtmlElement(indicator.element);


Does the element property for indicator objects still exist after removing the mount utility? See here: https://github.com/nasa/openmct/blob/vue-indicators/src/plugins/clearData/plugin.js#L34

Perhaps you need to create the indicator.element node here as well?

@shefalijoshi Not for most of our default indicators. The idea is that if the vueComponent is provided to the indicator API, it will mount the component directly. Otherwise, it will look for the element property and wrap it in an anonymous Vue component, and then mount that. So one or the other is required here. If both vueComponent and element are missing, then we fail fast.

OK. Add some documentation around this in the API change.

are the existing docs ok? this just explains how to use the API, the implementation details shouldn't be important to the average user?

scottbell

Looks great @ozyx - and much simpler! I had one question about the of -> on change that needs checking, but otherwise looks good.

Here are the new indicators in action:

Screen.Recording.2024-01-23.at.10.05.12.AM.mov

scottbell · 2024-01-23T08:47:42Z

e2e/tests/functional/plugins/lad/ladSet.e2e.spec.js

@@ -53,6 +53,9 @@ test.describe('LAD Table Sets', () => {

    await page.goto(ladTableSet.url);

+    // Wait for the initial value to show after mount
+    await expect(page.getByLabel('lad value').first()).not.toContainText('---');


yeah, seems rational here

src/plugins/notificationIndicator/components/NotificationIndicator.vue

src/ui/layout/status-bar/StatusIndicators.vue

src/utils/vueWrapHtmlElement.js

ozyx · 2024-01-23T16:09:12Z

Changes look good to me, thanks. Just needs some testing instructions.

Added testing instructions

scottbell

LGTM

…` assertion

ozyx added 7 commits January 19, 2024 21:21

refactor(indicators): use dynamic Vue components instead of mount()

9cd46ee

- Refactors some indicators to use Vue components directly as async components

refactor: use Vue reactivity for timestamps in clock indicator

21bb0bb

fix(test): fix unit tests and remove some console logs

5b155a1

test(e2e): stabilize ladSet e2e test

6019fc7

test: mix in some Vue indicators in indicatorSpec

2ccd6be

refactor: cleanup variable names

52f8603

ozyx added type:enhancement performance impacts or improves performance labels Jan 21, 2024

ozyx added this to the Target:3.3.0 milestone Jan 21, 2024

ozyx requested review from akhenry and scottbell January 21, 2024 19:04

docs: update IndicatorAPI docs

594e042

fix(e2e): wait for async status bar components to load before snapshot

71ee484

ozyx requested a review from unlikelyzero January 21, 2024 21:30

ozyx added 3 commits January 21, 2024 13:44

a11y(e2e): add aria-labels and wait for status bar to load

62dfffb

test(e2e): add exact: true

5acbbc0

fix: initializing indicators

f004bb4

unlikelyzero approved these changes Jan 22, 2024

View reviewed changes

ozyx added 5 commits January 21, 2024 17:26

fix(typo): uhhh.. how did that get there? O_o

6895f9e

fix: use synchronous components for default indicators

54c70b0

test: clean up, remove unnecessary nextTick()s

8f21f68

test: remove more nextTick()s

58aba3a

refactor: lint:fix

a5db40d

unlikelyzero modified the milestones: Target:3.3.0, Target:4.0.0 Jan 23, 2024

shefalijoshi reviewed Jan 23, 2024

View reviewed changes

scottbell requested changes Jan 23, 2024

View reviewed changes

fix: on -> off

84ecde1

ozyx requested a review from scottbell January 23, 2024 16:04

scottbell approved these changes Jan 23, 2024

View reviewed changes

Merge branch 'master' into vue-indicators

bcf16b2

ozyx added Target: VISTA R4.6 pr:e2e:couchdb npm run test:e2e:couchdb and removed Target: VISTA R4.6 labels Jan 23, 2024

ozyx enabled auto-merge (squash) January 23, 2024 16:59

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 23, 2024

test(e2e): stabilize tabs test

7178135

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 23, 2024

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 23, 2024

ozyx disabled auto-merge January 23, 2024 18:10

test(e2e): attempt to stabilize limit lines tests with `toHaveCount()…

9a7eb7d

…` assertion

akhenry approved these changes Jan 23, 2024

View reviewed changes

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 23, 2024

ozyx enabled auto-merge (squash) January 23, 2024 23:10

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 23, 2024

ozyx merged commit 1148644 into master Jan 23, 2024
21 checks passed

ozyx deleted the vue-indicators branch January 23, 2024 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#7394): Incorporate Status Indicators into the main Vue app #7395

feat(#7394): Incorporate Status Indicators into the main Vue app #7395

ozyx commented Jan 21, 2024 •

edited by scottbell

Loading

codecov bot commented Jan 21, 2024 •

edited

Loading

unlikelyzero left a comment

unlikelyzero Jan 22, 2024

ozyx Jan 22, 2024

scottbell Jan 23, 2024

akhenry commented Jan 23, 2024 •

edited

Loading

shefalijoshi Jan 23, 2024 •

edited

Loading

ozyx Jan 23, 2024 •

edited

Loading

shefalijoshi Jan 23, 2024

ozyx Jan 23, 2024

scottbell left a comment

scottbell Jan 23, 2024

ozyx commented Jan 23, 2024

scottbell left a comment

feat(#7394): Incorporate Status Indicators into the main Vue app #7395

feat(#7394): Incorporate Status Indicators into the main Vue app #7395

Conversation

ozyx commented Jan 21, 2024 • edited by scottbell Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

codecov bot commented Jan 21, 2024 • edited Loading

Codecov Report

unlikelyzero left a comment

Choose a reason for hiding this comment

unlikelyzero Jan 22, 2024

Choose a reason for hiding this comment

ozyx Jan 22, 2024

Choose a reason for hiding this comment

scottbell Jan 23, 2024

Choose a reason for hiding this comment

akhenry commented Jan 23, 2024 • edited Loading

shefalijoshi Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

ozyx Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

shefalijoshi Jan 23, 2024

Choose a reason for hiding this comment

ozyx Jan 23, 2024

Choose a reason for hiding this comment

scottbell left a comment

Choose a reason for hiding this comment

scottbell Jan 23, 2024

Choose a reason for hiding this comment

ozyx commented Jan 23, 2024

scottbell left a comment

Choose a reason for hiding this comment

ozyx commented Jan 21, 2024 •

edited by scottbell

Loading

codecov bot commented Jan 21, 2024 •

edited

Loading

akhenry commented Jan 23, 2024 •

edited

Loading

shefalijoshi Jan 23, 2024 •

edited

Loading

ozyx Jan 23, 2024 •

edited

Loading