fix(#7524): Open in New Tab action from a sub-object in a layout #7542

ozyx · 2024-03-03T19:51:01Z

Closes #7524
Closes #6982

Describe your changes:

Fixes the "Open in New Tab" action for sub-objects
- Removes logic that was meant for "data pivoting" feature to be able to pass time bound info to the main Open MCT app. Since "data pivoting" was moved to core Open MCT, this is no longer needed and was just causing crashes.
Greatly improves a11y of Menus and SuperMenus
Add .visibility-hidden global utility CSS class for providing accessible descriptions for screen-readers without affecting the view
Adds test for "Open in New Tab" from a display layout sub-object
Adds test data generation for display layout w/ sub-plot

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 Notable Change ? For example, will this break compatibility with existing APIs or projects which source 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.
Is this a breaking change to be called out in the release notes?
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?

codecov · 2024-03-03T19:58:45Z

Codecov Report

Attention: Patch coverage is 46.66667% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 55.38%. Comparing base (0eadc7a) to head (bfb2b66).

❗ Current head bfb2b66 differs from pull request most recent head e0645c4. Consider uploading reports for the commit e0645c4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7542      +/-   ##
==========================================
+ Coverage   53.96%   55.38%   +1.41%     
==========================================
  Files         672      672              
  Lines       27003    26996       -7     
  Branches     2624     2626       +2     
==========================================
+ Hits        14572    14951     +379     
+ Misses      11712    11326     -386     
  Partials      719      719

Flag	Coverage Δ
e2e-full	`23.70% <50.00%> (-0.04%)`	⬇️
e2e-stable	`59.76% <100.00%> (+6.69%)`	⬆️
unit	`48.52% <46.66%> (+0.07%)`	⬆️

Files	Coverage Δ
...tion/components/ExampleDataVisualizationSource.vue	`5.26% <ø> (+5.26%)`	⬆️
src/api/menu/components/MenuComponent.vue	`50.00% <ø> (ø)`
src/api/menu/components/SuperMenu.vue	`83.33% <ø> (+16.66%)`	⬆️
src/plugins/clock/components/ClockIndicator.vue	`75.00% <100.00%> (ø)`
...gins/inspectorDataVisualization/TelemetryFrame.vue	`0.00% <ø> (ø)`
...c/plugins/openInNewTabAction/openInNewTabAction.js	`100.00% <100.00%> (ø)`
...Conductor/independent/IndependentTimeConductor.vue	`0.00% <ø> (ø)`
src/tools/url.js	`100.00% <100.00%> (ø)`
src/ui/components/ObjectFrame.vue	`4.08% <ø> (-0.09%)`	⬇️
src/ui/layout/CreateButton.vue	`11.53% <100.00%> (ø)`
... and 9 more

... and 67 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 0eadc7a...e0645c4. Read the comment docs.

unlikelyzero · 2024-03-03T20:23:19Z

index.html

@@ -19,7 +19,7 @@
 this source code distribution or the Licensing information page available
 at runtime from the About dialog for additional information.
 -->
-<!DOCTYPE html>
+<!doctype html>


unlikelyzero · 2024-03-03T20:24:17Z

src/tools/urlSpec.js

@@ -65,15 +65,5 @@ describe('the url tool', function () {
      const constructedURL = objectPathToUrl(openmct, mockObjectPath);
      expect(constructedURL).toContain('#/browse/mock-parent-folder/mock-folder');
    });
-    it('can take params to set a custom url', () => {


do we have precise coverage for this

The code this tested was removed

JK. it's back now

unlikelyzero

Looks good so far. Let's take the time to understand what this means (from freecodecamp)

Know When to Use aria-describedby
Sometimes, we need to give more information to an element. For example, we might want to tell the user that the button they will press will open a new tab.

This information is important because the user needs to know where they are when navigating websites.

For these types of scenarios, we can use aria-describedby to give additional information.

Example:

⛔ BAD

<button aria-label="Close" aria-label="Opens in a new tab">	
Show related		
</button>

✅ GOOD

<button aria-label="Close" aria-describedby="description">
Show related		
</button>		
<div id="description">Opens in a new tab</div>

In the first example above, what engineers expect the screen reader to announce is: “button, Show related, opens in a new tab”.

But the screen reader does not do that. Instead, it says,“button, opens in a new tab”. The screen reader does not read the content inside, because aria-label always overrides the text content of the HTML5 element it has been applied to.

The second code snipped shows the correct way to use aria-describedby. The screen reader will read, “button, Show related, opens in a new tab”.

That information tells the user that the button is to show related content and if they press that button, it will navigate them to another tab.

Takeaway: Use aria-describedby to add additional information to elements.

unlikelyzero · 2024-03-04T14:30:52Z

e2e/appActions.js

  await expect(page.locator('.itc-popout')).toBeInViewport();
-
-  await setTimeBounds(page, startDate, endDate);
+  await setTimeBounds(page, start, end);


setTimeBoundsOfITC ?

it seems like it was designed to work for any time conductor? idk, I think all of these methods need an overhaul. not in this PR though.

unlikelyzero · 2024-03-04T14:31:37Z

e2e/tests/framework/appActions.e2e.spec.js

@@ -174,6 +174,6 @@ test.describe('AppActions', () => {
      type: 'Folder'
    });
    await openObjectTreeContextMenu(page, folder.url);
-    await expect(page.getByLabel('Menu')).toBeVisible();
+    await expect(page.getByLabel(`${folder.name} Context Menu`)).toBeVisible();


e2e/tests/functional/plugins/displayLayout/displayLayout.e2e.spec.js

unlikelyzero · 2024-03-04T14:36:02Z

e2e/tests/functional/plugins/displayLayout/displayLayout.e2e.spec.js

+    await newPage.waitForLoadState('domcontentloaded');
+
+    // Verify that the global time conductor bounds in the new page match the updated global bounds
+    expect(


ok this makes sense

unlikelyzero · 2024-03-04T15:16:36Z

src/api/menu/components/SuperMenu.vue

            :class="action.cssClass"
-            :title="action.description"


I think we may want to keep titles, just not use them for tests

Generally this is true, but in this case the title was basically pointless. The same contents are displayed on the right and read aloud by the screen reader. The title in this particular case served no purpose

unlikelyzero

Just two comments

ozyx · 2024-03-04T21:17:06Z

Have a few more changes after talking to @davetsay . Moving back to draft for now

- dataVisualization logic has moved from MMGIS plugin to the open source. As such, we can just use the time conductor bounds

This reverts commit 87aef35.

…ot + ITC enabled

- needs cleanup

This reverts commit b24c7da.

…lot in the inspector

This reverts commit 759ebd4.

davetsay · 2024-03-06T01:28:52Z

Looking good so far, @ozyx . @unlikelyzero must be losing his mind over this one.

davetsay · 2024-03-06T16:43:59Z

src/plugins/LADTable/components/LadRow.vue

@@ -260,7 +260,7 @@ export default {
        event.preventDefault();
        this.preview(this.objectPath);
      } else {
-        const resultUrl = identifierToString(this.openmct, this.objectPath);
+        const resultUrl = objectPathToUrl(this.openmct, this.objectPath);


right! we had a default export in addition to named exports in that file... scary

davetsay · 2024-03-07T16:49:46Z

src/plugins/inspectorDataVisualization/TelemetryFrame.vue

@@ -121,7 +121,8 @@ export default {
      };
      const newTabAction = this.openmct.actions.getAction('newTab');
      // No view context needed, so pass undefined.
-      // The urlParams arg will override the global time bounds with the dataviz plot bounds.
+      // The urlParams arg will override the global time bounds with the data visualization


src/plugins/inspectorDataVisualization/TelemetryFrame.vue

src/plugins/openInNewTabAction/openInNewTabAction.js

davetsay

one question / potential change. and I'm going to smoke test this before merging.

src/plugins/inspectorDataVisualization/TelemetryFrame.vue

davetsay

Great work.

ozyx added the type:bug label Mar 3, 2024

ozyx added this to the Target:4.0.0 milestone Mar 3, 2024

unlikelyzero reviewed Mar 3, 2024

View reviewed changes

ozyx marked this pull request as ready for review March 4, 2024 01:47

unlikelyzero reviewed Mar 4, 2024

View reviewed changes

e2e/tests/functional/plugins/displayLayout/displayLayout.e2e.spec.js Outdated Show resolved Hide resolved

unlikelyzero reviewed Mar 4, 2024

View reviewed changes

unlikelyzero approved these changes Mar 4, 2024

View reviewed changes

ozyx requested a review from davetsay March 4, 2024 17:21

ozyx marked this pull request as draft March 4, 2024 21:16

ozyx marked this pull request as ready for review March 5, 2024 02:38

ozyx added 13 commits March 4, 2024 18:41

refactor: url tools use named exports

652bf9e

fix: refactor method and remove customUrlParams

2afc8e7

test(e2e): verify bounds are preserved in data pivoting

4b5c16b

test: remove test as feature is no longer needed

3738297

- dataVisualization logic has moved from MMGIS plugin to the open source. As such, we can just use the time conductor bounds

refactor: autoformat keeps changing this so i'mma just commit it

c9ed24f

refactor: remove unnecessary code

e8fe59e

refactor: simplify, add docs

944e9fd

Revert "refactor: remove unnecessary code"

bf34b81

This reverts commit 87aef35.

a11y: improve aria labels for ITC

ef2db0c

fix: simplify url method

86ef367

fix: update ITC app actions

1293554

test: add test to generate test data for display layout w/ overlay pl…

70b8247

…ot + ITC enabled

test(e2e): add suite + test for open in new tab from subobject

768ecde

- needs cleanup

ozyx added 6 commits March 4, 2024 18:41

fix: no need for mount, use dynamic rendering instead

0e75c24

Revert "fix: simplify url method"

bc9f362

This reverts commit b24c7da.

fix: correct time conductor bounds when opening in a new tab from a p…

b849c3f

…lot in the inspector

test: fix e2e tests

6383a21

Revert "test: remove test as feature is no longer needed"

87b9866

This reverts commit 759ebd4.

test: move 2p annotation to test

28381b5

ozyx force-pushed the mct7524 branch from f2170f5 to 28381b5 Compare March 5, 2024 02:41

ozyx added 6 commits March 5, 2024 10:52

test: fix e2e

ff23585

fix: no words for the word god today

c48af0f

test: fix e2e

689f9da

fix: e2e test

6657b34

test: fix test

9b6cf51

driveby: fix perf test

6ab14b7

davetsay reviewed Mar 6, 2024

View reviewed changes

davetsay reviewed Mar 7, 2024

View reviewed changes

src/plugins/inspectorDataVisualization/TelemetryFrame.vue Show resolved Hide resolved

davetsay reviewed Mar 7, 2024

View reviewed changes

src/plugins/openInNewTabAction/openInNewTabAction.js Show resolved Hide resolved

davetsay requested changes Mar 7, 2024

View reviewed changes

src/plugins/inspectorDataVisualization/TelemetryFrame.vue Outdated Show resolved Hide resolved

fix: revert required prop change

06db9d0

akhenry requested a review from davetsay March 11, 2024 21:24

Merge branch 'master' into mct7524

bfb2b66

davetsay approved these changes Mar 11, 2024

View reviewed changes

davetsay added the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 11, 2024

davetsay enabled auto-merge (squash) March 11, 2024 23:14

Merge branch 'master' into mct7524

e0645c4

unlikelyzero disabled auto-merge March 11, 2024 23:39

unlikelyzero merged commit 8c2558b into master Mar 11, 2024
4 of 10 checks passed

unlikelyzero deleted the mct7524 branch March 11, 2024 23:39

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#7524): Open in New Tab action from a sub-object in a layout #7542

fix(#7524): Open in New Tab action from a sub-object in a layout #7542

ozyx commented Mar 3, 2024 •

edited

Loading

codecov bot commented Mar 3, 2024 •

edited

Loading

unlikelyzero Mar 3, 2024

ozyx Mar 3, 2024

unlikelyzero Mar 3, 2024

ozyx Mar 4, 2024

ozyx Mar 5, 2024

unlikelyzero left a comment •

edited

Loading

unlikelyzero Mar 4, 2024

ozyx Mar 4, 2024

unlikelyzero Mar 4, 2024

unlikelyzero Mar 4, 2024

unlikelyzero Mar 4, 2024

ozyx Mar 4, 2024

unlikelyzero left a comment

ozyx commented Mar 4, 2024

davetsay commented Mar 6, 2024

davetsay Mar 6, 2024

ozyx Mar 6, 2024

davetsay Mar 7, 2024

davetsay left a comment

davetsay left a comment

fix(#7524): Open in New Tab action from a sub-object in a layout #7542

fix(#7524): Open in New Tab action from a sub-object in a layout #7542

Conversation

ozyx commented Mar 3, 2024 • edited Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

codecov bot commented Mar 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unlikelyzero left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unlikelyzero left a comment

Choose a reason for hiding this comment

ozyx commented Mar 4, 2024

davetsay commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davetsay left a comment

Choose a reason for hiding this comment

davetsay left a comment

Choose a reason for hiding this comment

ozyx commented Mar 3, 2024 •

edited

Loading

codecov bot commented Mar 3, 2024 •

edited

Loading

unlikelyzero left a comment •

edited

Loading