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

[Inspector Tabs] Updates #7987

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

[Inspector Tabs] Updates #7987

wants to merge 49 commits into from

Conversation

jvigliotta
Copy link
Contributor

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:

  • 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 a notable change that will require a special callout in the release notes? For example, will this break compatibility with existing APIs or projects that consume these plugins?

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.
  • 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?

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 47.12644% with 46 lines in your changes missing coverage. Please review.

Project coverage is 23.88%. Comparing base (28b5d7c) to head (0deb2bf).

Files with missing lines Patch % Lines
...pectorViews/annotations/AnnotationsViewProvider.js 55.55% 8 Missing ⚠️
...stackedPlot/stackedPlotConfigurationInterceptor.js 12.50% 7 Missing ⚠️
src/api/telemetry/TelemetryAPI.js 28.57% 5 Missing ⚠️
...onditionWidget/conditionWidgetStylesInterceptor.js 33.33% 4 Missing ⚠️
...s/plot/overlayPlot/overlayPlotStylesInterceptor.js 33.33% 4 Missing ⚠️
src/plugins/LADTable/LADTableConfiguration.js 0.00% 3 Missing ⚠️
...s/telemetryTable/TableConfigurationViewProvider.js 0.00% 2 Missing ⚠️
.../telemetryTable/telemetryTableStylesInterceptor.js 50.00% 2 Missing ⚠️
...gins/LADTable/LADTableConfigurationViewProvider.js 0.00% 1 Missing ⚠️
src/plugins/LADTable/plugin.js 0.00% 1 Missing ⚠️
... and 9 more

❗ There is a different number of reports uploaded between BASE (28b5d7c) and HEAD (0deb2bf). Click for more details.

HEAD has 87 uploads less than BASE
Flag BASE (28b5d7c) HEAD (0deb2bf)
unit 32 0
e2e-full 48 1
e2e-ci 8 0
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     
Flag Coverage Δ
e2e-ci ?
e2e-full 23.88% <47.12%> (-18.13%) ⬇️
unit ?
Files with missing lines Coverage Δ
src/api/annotation/AnnotationAPI.js 19.44% <100.00%> (-72.11%) ⬇️
src/api/types/TypeRegistry.js 46.66% <100.00%> (-24.77%) ⬇️
src/plugins/displayLayout/DisplayLayoutType.js 33.33% <ø> (-66.67%) ⬇️
...rc/plugins/filters/FiltersInspectorViewProvider.js 44.44% <ø> (-51.21%) ⬇️
src/plugins/flexibleLayout/plugin.js 71.42% <ø> (-28.58%) ⬇️
src/plugins/gauge/GaugePlugin.js 22.22% <ø> (-19.45%) ⬇️
...ns/inspectorViews/elements/ElementsViewProvider.js 73.33% <100.00%> (-26.67%) ⬇️
...nspectorViews/properties/PropertiesViewProvider.js 100.00% <100.00%> (ø)
...spectorViews/styles/StylesInspectorViewProvider.js 83.33% <100.00%> (-16.67%) ⬇️
...c/plugins/interceptors/missingObjectInterceptor.js 57.14% <ø> (-42.86%) ⬇️
... and 24 more

... and 554 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 28b5d7c...0deb2bf. Read the comment docs.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- 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.
@charlesh88
Copy link
Contributor

charlesh88 commented Jan 16, 2025

Todos

@jvigliotta

  • Priority doesn't appear to update when switching from browse to edit - it's only triggering on navigation or sub-object selection. So if I'm in the main view of a composable object, and I click to edit it, I'd expect the priority of a tab to be evaluated and for the Inspector to update. We need this, for example, to allow Properties as high priority during browse, and low priority when editing.
  • The Styles tab is working oddly now in main views: it's now appearing for some domain objects. I have two Display Layouts and it appears for one but not the other:

Screenshot 2025-01-15 at 5 31 55 PM

Screenshot 2025-01-15 at 5 32 08 PM

  • Show Annotations for Overlay and Stacked plots.
  • Fix LAD Table Set Config tab problem.
  • Properties tab should not show for non-domain objects.
  • Add objectStyles to older domain objects that weren't previously initialized with them in their configuration.
  • Add objectStyles to initialize functions for view types that @charlesh88 can't figure out how to do:
    • Telemetry Table
  • Notebook should only show Annotations tab when an entry is selected, and as the first tab.

@charlesh88

  • Add objectStyles {} to stylable domain object initializers.

@charlesh88
Copy link
Contributor

Have pushed work:

  • Alphanumeric formatting tab set to default priority while editing, low priority during browse.
  • Good styling for Format tab contents in browse mode.
  • Make Elements pool visible in browse mode, omit edit capabilities.
  • Priorities set for Properties and Elements tabs in browse and edit modes: high/low and low/high, respectively.

jvigliotta and others added 6 commits January 16, 2025 08:50
… tab shows for them, setting it for older versions
- 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.
@charlesh88
Copy link
Contributor

charlesh88 commented Jan 17, 2025

Pushed:

  • Priorities set for Graph, Lad Table, Scatter Plot, Telem Tables and Time List views.
  • Changed several Inspector tab names to 'Config'; e2e tests updated where needed:
    • LAD Table.
    • Time List.
  • Created browse mode read-only Inspector views:
    • LAD Table, Lad Table set.
    • Telemetry Table.
  • Changed AnnotationsViewProvider.js canView to return false if editor.isEditing.
  • Added objectStyles: {} to initialize functions for multiple objects:
    • Condition Widget
    • Gauge
    • LAD Table
    • Overlay and Stacked Plots
  • 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.

- Added `objectStyles: {}` to initialize functions for multiple objects:
  - Condition Widget
  - Gauge
  - LAD Table
@jvigliotta jvigliotta changed the title Inspector Tabs Updates [Inspector Tabs] Updates Jan 22, 2025
@akhenry akhenry requested review from akhenry and removed request for shefalijoshi February 26, 2025 19:10
Copy link
Contributor

@akhenry akhenry left a 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 = [
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor

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.

So, this was the original spec for those views:
image

And here it is with updated priorities:
image

Copy link
Contributor Author

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!

@jvigliotta jvigliotta requested a review from akhenry March 3, 2025 23:49
@jvigliotta jvigliotta requested a review from akhenry March 6, 2025 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highest priority inspector tab is not auto selected
3 participants