-
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
[Inspector Tabs] Updates #7987
base: master
Are you sure you want to change the base?
[Inspector Tabs] Updates #7987
Conversation
…on annoatation tab
…urn annotatable types, cleaned up use of hasNumericTelemetry elsewhere in the code
Merging 2 inspector related branches for one PR.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7987 +/- ##
===========================================
- Coverage 57.59% 23.88% -33.72%
===========================================
Files 678 439 -239
Lines 27440 13674 -13766
Branches 2694 0 -2694
===========================================
- Hits 15805 3266 -12539
+ Misses 11292 10408 -884
+ Partials 343 0 -343
... and 554 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
- Alphanumeric formatting tab set to default priority while editing, low priority during browse. - Good styling for Format tab contents in browse mode. - Properties tab set to low priority during editing, default during browse.
- Make Elements pool visible in browse mode, omit edit capabilities.
- Edit and browse mode priorities for Properties and Elements.
- Adjusted edit and browse mode priorities for Properties and Elements. - Priority set for Gantt view.
Todos
|
Have pushed work:
|
… tab shows for them, setting it for older versions
…, keep it selected
- Priorities set for Graph, Lad Table, Scatter Plot, Telem Tables and Time List views. - Changed several Inspector tab names to 'Config'; tests and other code changed to target `key` instead of `name`: - LAD Table - Time List - will need regression testing for change noted re. `key` above. - Created browse mode read-only Inspector views: - LAD Table, Lad Table set. - Telemetry Table. and `showTab` functions; `showTab` has just been set to true for now. to prevent it from displaying when no filters can be set. - Changed AnnotationsViewProvider.js canView to return false if editor.isEditing. - Plot plugin.js now adds configuration.objectStyles {} for overlay and stacked plots on initialize. - FiltersView now displays a message for telem sources that don't have filter criteria available. - Code cleanup in FiltersInspectorViewProvider.js to remove metadata checks that never evaluated. - Annotations tab now set to never display when a view is being edited.
Pushed:
|
- Added `objectStyles: {}` to initialize functions for multiple objects: - Condition Widget - Gauge - LAD Table
…ith configurations
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.
Looks great, thanks! There are a number of places where we are initializing domainObject.configuration.objectStyles
in views because it doesn't exist on legacy objects.
We should never mutate objects in views unless something has changed. Supporting legacy objects is a special case where we should use an object interceptor to achieve this. The interceptor can augment object models once wherever they are used, even outside of the main views. There should be an interceptor for each type, and it should sit alongside the type that it augments in the code tree.
const objectSelection = selection?.[0]; | ||
const objectContext = objectSelection?.[0]?.context; | ||
const domainObject = objectContext?.item; | ||
const onlyEditMode = [ |
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.
These are views where we only want to show the style inspector in edit mode, is that correct? Why is that?
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.
My reason for trying to understand this is I'm concerned about the amount of plugin-specific code we are building into this view. I'd like to keep it as agnostic as possible, and have plugins opt-in to features like styling.
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'll let @charlesh88 answer this one!
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.
@akhenry I think the original idea was reducing the number of tabs in view while in browse mode. That said, I agree with reducing complexity have no real objection to showing the Style tab in browse mode for those view as long as it's prioritized properly in that mode, which will be different than in edit mode.
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'll make the update!
… get undefined objects
Closes #7442, #7959
Combination of two similar PR's
Describe your changes:
Whenever the selection changes, the first tab (highest priority) is auto selected. There may be some cases (editing styles) when we'd want that tab to stay selected as well. Also updates when and where the tabs show based on the items being viewed.
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist