-
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
[Search] SearchProvider and Tree Search enhancements/fixes #3400
Conversation
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 callout so far
platform/search/bundle.js
Outdated
@@ -109,7 +109,8 @@ define([ | |||
"topic", | |||
"GENERIC_SEARCH_ROOTS", | |||
"USE_LEGACY_INDEXER", | |||
"openmct" | |||
"openmct", | |||
"objectService" |
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.
lets add this in place of modelService, since we are replacing it
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.
updated!
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.
LGTM
@jvigliotta - were there any legacy tests that need updating? We should not be adding tests to legacy code but we should update them if needed |
Reviewer ChecklistChanges appear to address issue? Yes |
Added the objectService to the GenericSearchProvider to handle cases where the wrong provider was being used to get the object/model. Also small refactor to the navigation tree to handle search more appropriately (searching indicator, clearing children, etc).
closes #3399
Author Checklist
Changes address original issue? Yes
Unit tests included and/or updated with changes? N/A Legacy
Command line build passes? Yes
Changes have been smoke-tested? Yes
Testing instructions included? Yes