Add Application Page Header for Visualize Pages #7712

LDrago27 · 2024-08-14T16:36:37Z

Description

This changes at resolving following issues.

Add New Application Header for Visualize when the newHomePage flag is done
Show New Application Header Layout for Discover when newHomePage and queryEnhancement flag is true.
Show New Application Header Layout on Visualize landing page when the newHomePage flag is done
Show New Application Header Layout on Dashboards landing Page when the newHomePage flag is done
Add New Application Header for Vis Builder when the newHomePage flag is done

Issues Resolved

Screenshot

Visualize with newHomePage flag is done

Discover when newHomePage and queryEnhancement flag is true

Visualize landing page

Dashboards Landing page

Vis Builder Page

Testing the changes

Application header in Visualize

Enable the new homePage flag in Advanced Settings
Navigate to the visualize Page and Check the new Page headers
Validate the new changes

Application Header layout in Discover

Enable the new homePage flag in Advanced Settings
Enable the query Enhancement flag in Advanced Settings
Validate the new changes

Changelog

feat: Add New Page Header to Visualize

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

codecov · 2024-08-14T16:58:38Z

Codecov Report

Attention: Patch coverage is 8.69565% with 42 lines in your changes missing coverage. Please review.

Project coverage is 63.80%. Comparing base (9a84202) to head (b0252ad).
Report is 185 commits behind head on main.

Files with missing lines	Patch %	Lines
...er/public/application/utils/get_top_nav_config.tsx	14.28%	18 Missing ⚠️
...components/dashboard_listing/dashboard_listing.tsx	0.00%	10 Missing ⚠️
src/plugins/vis_builder/public/application/app.tsx	0.00%	6 Missing ⚠️
..._builder/public/application/components/top_nav.tsx	0.00%	4 Missing ⚠️
.../data_explorer/public/components/app_container.tsx	33.33%	2 Missing ⚠️
...s_react/public/table_list_view/table_list_view.tsx	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7712      +/-   ##
==========================================
- Coverage   63.82%   63.80%   -0.03%     
==========================================
  Files        3653     3655       +2     
  Lines       81038    81157     +119     
  Branches    12903    12933      +30     
==========================================
+ Hits        51726    51784      +58     
- Misses      26138    26194      +56     
- Partials     3174     3179       +5

Flag	Coverage Δ
Linux_1	`30.07% <6.97%> (+0.01%)`	⬆️
Linux_2	`55.87% <ø> (+0.01%)`	⬆️
Linux_3	`40.41% <6.66%> (-0.01%)`	⬇️
Linux_4	`31.29% <0.00%> (+<0.01%)`	⬆️
Windows_1	`30.08% <6.97%> (-0.01%)`	⬇️
Windows_2	`55.82% <ø> (+0.01%)`	⬆️
Windows_3	`40.42% <6.66%> (?)`
Windows_4	`31.29% <0.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Suchit Sahoo <[email protected]>

virajsanghvi

May want to address Miki's feedback + patch coverage, but otherwise lgtm

Signed-off-by: Suchit Sahoo <[email protected]>

AMoo-Miki · 2024-08-20T23:16:33Z

src/plugins/vis_builder/public/application/utils/get_top_nav_config.tsx

+        : i18n.translate('visBuilder.topNavMenu.saveVisualizationButtonLabel', {
+            defaultMessage: 'save',
+          }),
+      ariaLabel: i18n.translate('visBuilder.topNavMenu.saveVisualizationAsButtonLabel', {


Check with UX but I feel when disabled, the aria-label might want to show the reason too.

Sure will check with UX Regarding this.

AMoo-Miki · 2024-08-20T23:17:08Z

src/plugins/visualize/public/application/app.tsx

@@ -35,6 +35,7 @@ import { Route, Switch, useLocation } from 'react-router-dom';
 import { AppMountParameters } from 'opensearch-dashboards/public';
 import { syncQueryStateWithUrl } from '../../../data/public';
 import { useOpenSearchDashboards } from '../../../opensearch_dashboards_react/public';
+import { HeaderVariant } from '../../../../core/public/index';


is this needed?

Nope I will clean it up.

AMoo-Miki · 2024-08-20T23:18:56Z

src/plugins/visualize/public/application/components/visualize_byvalue_editor.tsx

+  useEffect(() => {
+    if (showActionsInGroup) setHeaderVariant?.(HeaderVariant.APPLICATION);
+
+    return () => {
+      setHeaderVariant?.();
+    };


If you are doing this on most of the pages, you can actually set the default header while registering the app to use APPLICATION and then set the variant to page on the page that needs it.

I tried the method you suggested, there are some issues arising during navigations i.e when I change across the listing page and the individual editor pages.

AMoo-Miki · 2024-08-20T23:20:25Z

src/plugins/visualize/public/application/components/visualize_top_nav.tsx

@@ -216,14 +259,15 @@ const TopNav = ({
      savedQueryId={currentAppState.savedQuery}
      onSavedQueryIdChange={stateContainer.transitions.updateSavedQuery}
      indexPatterns={indexPatterns}
-      screenTitle={vis.title}
+      screenTitle={vis.title.length ?? '' ? vis.title : 'New visualization'}


Why not just

Suggested change

screenTitle={vis.title.length ?? '' ? vis.title : 'New visualization'}

screenTitle={vis.title || 'New visualization'}

Will update it

* Add Application Header for Visualize Signed-off-by: Suchit Sahoo <[email protected]> * Fix Application Header Layout for Discover Signed-off-by: Suchit Sahoo <[email protected]> * Changeset file for PR #7712 created/updated * Add new Page Header in Vis Builder Signed-off-by: Suchit Sahoo <[email protected]> --------- Signed-off-by: Suchit Sahoo <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit b3234dd) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Add Application Header for Visualize * Fix Application Header Layout for Discover * Changeset file for PR #7712 created/updated * Add new Page Header in Vis Builder --------- (cherry picked from commit b3234dd) Signed-off-by: Suchit Sahoo <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

github-actions bot added the seasoned-contributor label Aug 14, 2024

opensearch-changeset-bot bot added a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Aug 14, 2024

Changeset file for PR opensearch-project#7712 created/updated

5c82c05

LDrago27 force-pushed the visualizeNewHeader branch 4 times, most recently from 47646b4 to 87c2fcb Compare August 16, 2024 17:30

LDrago27 marked this pull request as ready for review August 16, 2024 17:30

AMoo-Miki previously approved these changes Aug 16, 2024

View reviewed changes

ashwin-pc added backport 2.x v2.17.0 look & feel Look and Feel Improvements labels Aug 16, 2024

LDrago27 and others added 3 commits August 19, 2024 20:45

Add Application Header for Visualize

0ed7d1a

Signed-off-by: Suchit Sahoo <[email protected]>

Fix Application Header Layout for Discover

4e14a01

Signed-off-by: Suchit Sahoo <[email protected]>

Changeset file for PR opensearch-project#7712 created/updated

fb65fcb

virajsanghvi previously approved these changes Aug 19, 2024

View reviewed changes

LDrago27 dismissed stale reviews from virajsanghvi and AMoo-Miki via 95c9f8c August 19, 2024 23:19

LDrago27 force-pushed the visualizeNewHeader branch 2 times, most recently from 95c9f8c to f0e2d66 Compare August 19, 2024 23:40

LDrago27 changed the title ~~Add Application Page Header for Visualize~~ Add Application Page Header for Visualize Pages Aug 20, 2024

LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Aug 20, 2024

Changeset file for PR opensearch-project#7712 created/updated

7bbdbce

LDrago27 force-pushed the visualizeNewHeader branch from f0e2d66 to ac27eca Compare August 20, 2024 03:05

LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Aug 20, 2024

Changeset file for PR opensearch-project#7712 created/updated

feafcf8

LDrago27 force-pushed the visualizeNewHeader branch from ac27eca to 79ff126 Compare August 20, 2024 18:57

Add new Page Header in Vis Builder

b0252ad

Signed-off-by: Suchit Sahoo <[email protected]>

LDrago27 force-pushed the visualizeNewHeader branch from 79ff126 to b0252ad Compare August 20, 2024 22:07

virajsanghvi approved these changes Aug 20, 2024

View reviewed changes

AMoo-Miki reviewed Aug 20, 2024

View reviewed changes

AMoo-Miki approved these changes Aug 20, 2024

View reviewed changes

LDrago27 merged commit b3234dd into opensearch-project:main Aug 20, 2024
66 of 67 checks passed

opensearch-trigger-bot bot mentioned this pull request Aug 20, 2024

[Backport 2.x] Add Application Page Header for Visualize Pages #7778

Merged

AMoo-Miki added the in-next label Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Application Page Header for Visualize Pages #7712

Add Application Page Header for Visualize Pages #7712

LDrago27 commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

virajsanghvi left a comment

AMoo-Miki Aug 20, 2024

LDrago27 Aug 20, 2024

AMoo-Miki Aug 20, 2024

LDrago27 Aug 20, 2024

AMoo-Miki Aug 20, 2024

LDrago27 Aug 20, 2024

AMoo-Miki Aug 20, 2024

LDrago27 Aug 20, 2024 •

edited

Loading

	screenTitle={vis.title.length ?? '' ? vis.title : 'New visualization'}
	screenTitle={vis.title \|\| 'New visualization'}

Add Application Page Header for Visualize Pages #7712

Add Application Page Header for Visualize Pages #7712

Conversation

LDrago27 commented Aug 14, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

virajsanghvi left a comment

Choose a reason for hiding this comment

AMoo-Miki Aug 20, 2024

Choose a reason for hiding this comment

LDrago27 Aug 20, 2024

Choose a reason for hiding this comment

AMoo-Miki Aug 20, 2024

Choose a reason for hiding this comment

LDrago27 Aug 20, 2024

Choose a reason for hiding this comment

AMoo-Miki Aug 20, 2024

Choose a reason for hiding this comment

LDrago27 Aug 20, 2024

Choose a reason for hiding this comment

AMoo-Miki Aug 20, 2024

Choose a reason for hiding this comment

LDrago27 Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

LDrago27 commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

LDrago27 Aug 20, 2024 •

edited

Loading