-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: Mission Status for Situational Awareness #7418
Conversation
* at runtime from the About dialog for additional information. | ||
*****************************************************************************/ | ||
/* eslint-disable func-style */ | ||
import { onBeforeUnmount, reactive } from 'vue'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Composables for ResizeObservers and window resizes
* @param {string} event - The name of the event to listen for. | ||
* @param {Function} callback - The callback function to execute when the event is triggered. | ||
*/ | ||
export function useEventListener(target, event, handler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generic composables for EventListeners and EventEmitters-- these can be used to construct more complex composables (such as the window resize composable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we don't have to track on
and off
anymore! good stuff
setup() { | ||
const { windowSize } = useWindowResize(); | ||
const isPopupVisible = ref(false); | ||
const userIndicatorRef = ref(null); | ||
const popupRef = ref(null); | ||
|
||
// eslint-disable-next-line func-style | ||
const handleOutsideClick = (event) => { | ||
if (isPopupVisible.value && popupRef.value && !popupRef.value.contains(event.target)) { | ||
isPopupVisible.value = false; | ||
} | ||
}; | ||
|
||
useEventListener(document, 'click', handleOutsideClick); | ||
|
||
return { windowSize, isPopupVisible, popupRef, userIndicatorRef }; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompositionAPI syntax here so we can use reactive refs and composables
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #7418 +/- ##
==========================================
+ Coverage 54.66% 55.36% +0.69%
==========================================
Files 668 671 +3
Lines 26800 26919 +119
Branches 2606 2614 +8
==========================================
+ Hits 14651 14903 +252
+ Misses 11433 11300 -133
Partials 716 716
... and 41 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
<Teleport to="body"> | ||
<div v-show="isPopupVisible" ref="popupRef" class="c-user-control-panel" :style="popupStyle"> | ||
<MissionStatusPopup v-show="canSetMissionStatus" @dismiss="togglePopup" /> | ||
</div> | ||
</Teleport> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually this logic can be made more generic and move into some kind of higher-order function that can apply auto-positioning popups to whatever component we pass in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good @ozyx
inject: ['openmct'], | ||
inheritAttrs: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fragments!!
* @param {string} event - The name of the event to listen for. | ||
* @param {Function} callback - The callback function to execute when the event is triggered. | ||
*/ | ||
export function useEventListener(target, event, handler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we don't have to track on
and off
anymore! good stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working through the .vue
popup changes. Will get back to this soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that the vue changes are more of a stub than actual changes. Will come back to this when it's out of draft.
- These tests are going to be wonky since they depend on the View. Any unit tests depending on Vue / the View will become increasingly volatile over time as we migrate more of the app into the main Vue app. Since these LOC are already covered by e2e, I'm going to remove them. We will need to move towards a more component / Vue-friendly testing framework to stabilize all of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small changes, but looks good overall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some notes from testing plus a couple other finds.
|
||
try { | ||
// Listen for missionActionStatusChange events | ||
useEventEmitter(openmct.user.status, 'missionActionStatusChange', ({ action, status }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handler parameter signature does not match the what is emitted by the StatusAPI here:
https://github.com/nasa/openmct/pull/7418/files#diff-de4973d1b097ba7bca4bd6b61530a0edee5925fd20eb9fb537a1bd133d474119R348
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still seeing this issue testing locally. Will check on the banner deployment and resolve if it's OK
Extracted and created issue #7444 |
|
||
try { | ||
// Listen for missionActionStatusChange events | ||
useEventEmitter(openmct.user.status, 'missionActionStatusChange', ({ action, status }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one bug remains that could be a user/local testing issue. The rest looks good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs tests (probably create an issue for this). Approving with caveats.
Closes VIPEROMCT-461
Closes #7420
Describe your changes:
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist