Skip to content
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

Incorporate IndicatorAPI into the Vue reactivity system #7394

Closed
ozyx opened this issue Jan 21, 2024 · 4 comments · Fixed by #7395
Closed

Incorporate IndicatorAPI into the Vue reactivity system #7394

ozyx opened this issue Jan 21, 2024 · 4 comments · Fixed by #7395
Labels
performance impacts or improves performance type:enhancement verified Tested or intentionally closed
Milestone

Comments

@ozyx
Copy link
Member

ozyx commented Jan 21, 2024

Is your feature request related to a problem? Please describe.
In our IndicatorAPI we expect an HTMLElement in order to "go around" the Vue system and:

  1. Mount HTMLElements dynamically (using this.$el.appendChild)
  2. Maintain the framework-agnostic nature of the Open MCT API

This approach works, but can lose reactivity if we're not careful. Additionally, we can't conditionally render/un-render certain indicators based on API-wide settings such as User Role. This approach also has performance implications, since we create an Anonymous Vue component and mount an additional Vue app for each indicator.

Describe the solution you'd like
image

We should avoid using mount() and creating a whole new Vue app for each indicator.

With Vue 3, there are better ways to wrap HTMLElements as Vue components (using render functions) so we can maintain Open MCT's framework-agnostic nature, while also keeping the indicators within the same main Vue app. Additionally, we can support rendering Vue components directly via a new, optional property on indicators.

@ozyx ozyx added type:enhancement performance impacts or improves performance labels Jan 21, 2024
@ozyx ozyx self-assigned this Jan 21, 2024
@akhenry
Copy link
Contributor

akhenry commented Jan 23, 2024

vue

@ozyx
Copy link
Member Author

ozyx commented Jan 23, 2024

Hah! I realized this after I made the meme so I let it slide :)

WHAT IF I TOLD YOU

WE CAN (eventually) HAVE ONLY ONE VUE APP

I guess would be the correct text

@ozyx
Copy link
Member Author

ozyx commented Jan 23, 2024

Testing Instructions

  1. Load Open MCT locally in its default configuration
  2. Verify that all default Indicators appear and are functional (snapshot, clock, clearData, notifications)
  3. Load Open MCT VIPER
  4. Verify that all indicators appear and are functional (indicators listed above, plus Couch DB status, etc.)
  5. Open the console and install the Performance Indicator by using the command: this.openmct.install(this.openmct.plugins.PerformanceIndicator())
  6. Verify it appears in the status bar immediately and dynamically updates

ozyx added a commit that referenced this issue Jan 23, 2024
* feat(IndicatorAPI): accept Vue components

- 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).

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

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

* refactor: use Vue reactivity for timestamps in clock indicator

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

* test(e2e): stabilize ladSet e2e test

* test: mix in some Vue indicators in indicatorSpec

* refactor: cleanup variable names

* docs: update IndicatorAPI docs

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

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

* test(e2e): add exact: true

* fix: initializing indicators

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

* fix: use synchronous components for default indicators

* test: clean up, remove unnecessary `nextTick()`s

* test: remove more `nextTick()`s

* refactor: lint:fix

* fix: `on` -> `off`

* test(e2e): stabilize tabs test

* test(e2e): attempt to stabilize limit lines tests with `toHaveCount()` assertion
@unlikelyzero unlikelyzero added this to the Target:4.0.0 milestone Jan 25, 2024
@davetsay
Copy link
Contributor

davetsay commented Feb 8, 2024

verified

@ozyx ozyx added the verified Tested or intentionally closed label Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance impacts or improves performance type:enhancement verified Tested or intentionally closed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants