Skip to content

Commit f4dfa30

Browse files
yuye-awsruanyl
yuye-aws
and
ruanyl
authored
[Backport workspace] Left navigation menu adjustment (opensearch-project#192) (opensearch-project#233)
* [Workspace][Feature] Left navigation menu adjustment (opensearch-project#192) * add util function to filter workspace feature by wildcard Signed-off-by: Yulong Ruan <[email protected]> * resolve conflict Signed-off-by: yuye-aws <[email protected]> * update tests and snapshots Signed-off-by: yuye-aws <[email protected]> * small adjustment to left menu Signed-off-by: yuye-aws <[email protected]> * resolve git conflict Signed-off-by: yuye-aws <[email protected]> * rename nav link service function Signed-off-by: yuye-aws <[email protected]> * unit test for workspace plugin.ts Signed-off-by: yuye-aws <[email protected]> * update snapshots Signed-off-by: yuye-aws <[email protected]> * optimize code Signed-off-by: yuye-aws <[email protected]> * optimize code Signed-off-by: yuye-aws <[email protected]> * optimize code Signed-off-by: yuye-aws <[email protected]> * optimize code Signed-off-by: yuye-aws <[email protected]> * optimize code Signed-off-by: yuye-aws <[email protected]> * optimize code Signed-off-by: yuye-aws <[email protected]> --------- Signed-off-by: Yulong Ruan <[email protected]> Signed-off-by: yuye-aws <[email protected]> Co-authored-by: Yulong Ruan <[email protected]> * resolve conflict Signed-off-by: yuye-aws <[email protected]> * update snapshots Signed-off-by: yuye-aws <[email protected]> * resolve conflicts Signed-off-by: yuye-aws <[email protected]> * restore snapshot Signed-off-by: yuye-aws <[email protected]> * update tests Signed-off-by: yuye-aws <[email protected]> --------- Signed-off-by: Yulong Ruan <[email protected]> Signed-off-by: yuye-aws <[email protected]> Co-authored-by: Yulong Ruan <[email protected]>
1 parent 7e2ace0 commit f4dfa30

File tree

17 files changed

+1316
-487
lines changed

17 files changed

+1316
-487
lines changed

‎src/core/public/chrome/chrome_service.mock.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ const createStartContractMock = () => {
4343
const startContract: DeeplyMockedKeys<InternalChromeStart> = {
4444
getHeaderComponent: jest.fn(),
4545
navLinks: {
46+
setNavLinks: jest.fn(),
4647
getNavLinks$: jest.fn(),
47-
getFilteredNavLinks$: jest.fn(),
48-
setFilteredNavLinks: jest.fn(),
48+
getAllNavLinks$: jest.fn(),
4949
has: jest.fn(),
5050
get: jest.fn(),
5151
getAll: jest.fn(),

‎src/core/public/chrome/chrome_service.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ export class ChromeService {
277277
homeHref={http.basePath.prepend('/app/home')}
278278
isVisible$={this.isVisible$}
279279
opensearchDashboardsVersion={injectedMetadata.getOpenSearchDashboardsVersion()}
280-
navLinks$={navLinks.getFilteredNavLinks$()}
280+
navLinks$={navLinks.getNavLinks$()}
281281
customNavLink$={customNavLink$.pipe(takeUntil(this.stop$))}
282282
recentlyAccessed$={recentlyAccessed.get$()}
283283
navControlsLeft$={navControls.getLeft$()}

‎src/core/public/chrome/nav_links/nav_links_service.test.ts‎

Lines changed: 126 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,18 +32,12 @@ import { NavLinksService } from './nav_links_service';
3232
import { take, map, takeLast } from 'rxjs/operators';
3333
import { App } from '../../application';
3434
import { BehaviorSubject } from 'rxjs';
35+
import { ChromeNavLink } from 'opensearch-dashboards/public';
3536

3637
const availableApps = new Map([
3738
['app1', { id: 'app1', order: 0, title: 'App 1', icon: 'app1' }],
38-
[
39-
'app2',
40-
{
41-
id: 'app2',
42-
order: -10,
43-
title: 'App 2',
44-
euiIconType: 'canvasApp',
45-
},
46-
],
39+
['app2', { id: 'app2', order: -10, title: 'App 2', euiIconType: 'canvasApp' }],
40+
['app3', { id: 'app3', order: 10, title: 'App 3', icon: 'app3' }],
4741
['chromelessApp', { id: 'chromelessApp', order: 20, title: 'Chromless App', chromeless: true }],
4842
]);
4943

@@ -66,7 +60,110 @@ describe('NavLinksService', () => {
6660
start = service.start({ application: mockAppService, http: mockHttp });
6761
});
6862

69-
describe('#getNavLinks$()', () => {
63+
describe('#getAllNavLinks$()', () => {
64+
it('does not include `chromeless` applications', async () => {
65+
expect(
66+
await start
67+
.getAllNavLinks$()
68+
.pipe(
69+
take(1),
70+
map((links) => links.map((l) => l.id))
71+
)
72+
.toPromise()
73+
).not.toContain('chromelessApp');
74+
});
75+
76+
it('sorts navLinks by `order` property', async () => {
77+
expect(
78+
await start
79+
.getAllNavLinks$()
80+
.pipe(
81+
take(1),
82+
map((links) => links.map((l) => l.id))
83+
)
84+
.toPromise()
85+
).toEqual(['app2', 'app1', 'app3']);
86+
});
87+
88+
it('emits multiple values', async () => {
89+
const navLinkIds$ = start.getAllNavLinks$().pipe(map((links) => links.map((l) => l.id)));
90+
const emittedLinks: string[][] = [];
91+
navLinkIds$.subscribe((r) => emittedLinks.push(r));
92+
start.update('app1', { href: '/foo' });
93+
94+
service.stop();
95+
expect(emittedLinks).toEqual([
96+
['app2', 'app1', 'app3'],
97+
['app2', 'app1', 'app3'],
98+
]);
99+
});
100+
101+
it('completes when service is stopped', async () => {
102+
const last$ = start.getAllNavLinks$().pipe(takeLast(1)).toPromise();
103+
service.stop();
104+
await expect(last$).resolves.toBeInstanceOf(Array);
105+
});
106+
});
107+
108+
describe('#getNavLinks$() when non null', () => {
109+
// set nav links, nav link with order smaller than 0 will be filtered
110+
beforeEach(() => {
111+
const navLinks = new Map<string, ChromeNavLink>();
112+
start.getAllNavLinks$().subscribe((links) =>
113+
links.forEach((link) => {
114+
if (link.order !== undefined && link.order >= 0) {
115+
navLinks.set(link.id, link);
116+
}
117+
})
118+
);
119+
start.setNavLinks(navLinks);
120+
});
121+
122+
it('does not include `app2` applications', async () => {
123+
expect(
124+
await start
125+
.getNavLinks$()
126+
.pipe(
127+
take(1),
128+
map((links) => links.map((l) => l.id))
129+
)
130+
.toPromise()
131+
).not.toContain('app2');
132+
});
133+
134+
it('sorts navLinks by `order` property', async () => {
135+
expect(
136+
await start
137+
.getNavLinks$()
138+
.pipe(
139+
take(1),
140+
map((links) => links.map((l) => l.id))
141+
)
142+
.toPromise()
143+
).toEqual(['app1', 'app3']);
144+
});
145+
146+
it('emits multiple values', async () => {
147+
const navLinkIds$ = start.getNavLinks$().pipe(map((links) => links.map((l) => l.id)));
148+
const emittedLinks: string[][] = [];
149+
navLinkIds$.subscribe((r) => emittedLinks.push(r));
150+
start.update('app1', { href: '/foo' });
151+
152+
service.stop();
153+
expect(emittedLinks).toEqual([
154+
['app1', 'app3'],
155+
['app1', 'app3'],
156+
]);
157+
});
158+
159+
it('completes when service is stopped', async () => {
160+
const last$ = start.getNavLinks$().pipe(takeLast(1)).toPromise();
161+
service.stop();
162+
await expect(last$).resolves.toBeInstanceOf(Array);
163+
});
164+
});
165+
166+
describe('#getNavLinks$() when null', () => {
70167
it('does not include `chromeless` applications', async () => {
71168
expect(
72169
await start
@@ -79,7 +176,19 @@ describe('NavLinksService', () => {
79176
).not.toContain('chromelessApp');
80177
});
81178

82-
it('sorts navlinks by `order` property', async () => {
179+
it('include `app2` applications', async () => {
180+
expect(
181+
await start
182+
.getNavLinks$()
183+
.pipe(
184+
take(1),
185+
map((links) => links.map((l) => l.id))
186+
)
187+
.toPromise()
188+
).toContain('app2');
189+
});
190+
191+
it('sorts navLinks by `order` property', async () => {
83192
expect(
84193
await start
85194
.getNavLinks$()
@@ -88,7 +197,7 @@ describe('NavLinksService', () => {
88197
map((links) => links.map((l) => l.id))
89198
)
90199
.toPromise()
91-
).toEqual(['app2', 'app1']);
200+
).toEqual(['app2', 'app1', 'app3']);
92201
});
93202

94203
it('emits multiple values', async () => {
@@ -99,8 +208,8 @@ describe('NavLinksService', () => {
99208

100209
service.stop();
101210
expect(emittedLinks).toEqual([
102-
['app2', 'app1'],
103-
['app2', 'app1'],
211+
['app2', 'app1', 'app3'],
212+
['app2', 'app1', 'app3'],
104213
]);
105214
});
106215

@@ -123,7 +232,7 @@ describe('NavLinksService', () => {
123232

124233
describe('#getAll()', () => {
125234
it('returns a sorted array of navlinks', () => {
126-
expect(start.getAll().map((l) => l.id)).toEqual(['app2', 'app1']);
235+
expect(start.getAll().map((l) => l.id)).toEqual(['app2', 'app1', 'app3']);
127236
});
128237
});
129238

@@ -148,7 +257,7 @@ describe('NavLinksService', () => {
148257
map((links) => links.map((l) => l.id))
149258
)
150259
.toPromise()
151-
).toEqual(['app2', 'app1']);
260+
).toEqual(['app2', 'app1', 'app3']);
152261
});
153262

154263
it('does nothing on chromeless applications', async () => {
@@ -161,7 +270,7 @@ describe('NavLinksService', () => {
161270
map((links) => links.map((l) => l.id))
162271
)
163272
.toPromise()
164-
).toEqual(['app2', 'app1']);
273+
).toEqual(['app2', 'app1', 'app3']);
165274
});
166275

167276
it('removes all other links', async () => {

‎src/core/public/chrome/nav_links/nav_links_service.ts‎

Lines changed: 24 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,14 @@ export interface ChromeNavLinks {
5454
getNavLinks$(): Observable<Array<Readonly<ChromeNavLink>>>;
5555

5656
/**
57-
* Set an observable for a sorted list of filtered navlinks.
57+
* Get an observable for a sorted list of all navlinks.
5858
*/
59-
getFilteredNavLinks$(): Observable<Array<Readonly<ChromeNavLink>>>;
59+
getAllNavLinks$(): Observable<Array<Readonly<ChromeNavLink>>>;
6060

6161
/**
62-
* Set filtered navlinks.
62+
* Set navlinks.
6363
*/
64-
setFilteredNavLinks(filteredNavLinks: ReadonlyMap<string, ChromeNavLink>): void;
64+
setNavLinks(navLinks: ReadonlyMap<string, ChromeNavLink>): void;
6565

6666
/**
6767
* Get the state of a navlink at this point in time.
@@ -126,9 +126,6 @@ type LinksUpdater = (navLinks: Map<string, NavLinkWrapper>) => Map<string, NavLi
126126

127127
export class NavLinksService {
128128
private readonly stop$ = new ReplaySubject(1);
129-
private filteredNavLinks$ = new BehaviorSubject<ReadonlyMap<string, ChromeNavLink> | undefined>(
130-
undefined
131-
);
132129

133130
public start({ application, http }: StartDeps): ChromeNavLinks {
134131
const appLinks$ = application.applications$.pipe(
@@ -145,7 +142,10 @@ export class NavLinksService {
145142
// manual link modifications to be able to re-apply then after every
146143
// availableApps$ changes.
147144
const linkUpdaters$ = new BehaviorSubject<LinksUpdater[]>([]);
148-
const navLinks$ = new BehaviorSubject<ReadonlyMap<string, NavLinkWrapper>>(new Map());
145+
const displayedNavLinks$ = new BehaviorSubject<ReadonlyMap<string, ChromeNavLink> | undefined>(
146+
undefined
147+
);
148+
const allNavLinks$ = new BehaviorSubject<ReadonlyMap<string, NavLinkWrapper>>(new Map());
149149

150150
combineLatest([appLinks$, linkUpdaters$])
151151
.pipe(
@@ -154,42 +154,40 @@ export class NavLinksService {
154154
})
155155
)
156156
.subscribe((navLinks) => {
157-
navLinks$.next(navLinks);
157+
allNavLinks$.next(navLinks);
158158
});
159159

160160
const forceAppSwitcherNavigation$ = new BehaviorSubject(false);
161161

162162
return {
163163
getNavLinks$: () => {
164-
return navLinks$.pipe(map(sortNavLinks), takeUntil(this.stop$));
164+
return combineLatest([allNavLinks$, displayedNavLinks$]).pipe(
165+
map(([allNavLinks, displayedNavLinks]) =>
166+
displayedNavLinks === undefined ? sortLinks(allNavLinks) : sortLinks(displayedNavLinks)
167+
),
168+
takeUntil(this.stop$)
169+
);
165170
},
166171

167-
setFilteredNavLinks: (filteredNavLinks: ReadonlyMap<string, ChromeNavLink>) => {
168-
this.filteredNavLinks$.next(filteredNavLinks);
172+
setNavLinks: (navLinks: ReadonlyMap<string, ChromeNavLink>) => {
173+
displayedNavLinks$.next(navLinks);
169174
},
170175

171-
getFilteredNavLinks$: () => {
172-
return combineLatest([navLinks$, this.filteredNavLinks$]).pipe(
173-
map(([navLinks, filteredNavLinks]) =>
174-
filteredNavLinks === undefined
175-
? sortNavLinks(navLinks)
176-
: sortChromeNavLinks(filteredNavLinks)
177-
),
178-
takeUntil(this.stop$)
179-
);
176+
getAllNavLinks$: () => {
177+
return allNavLinks$.pipe(map(sortLinks), takeUntil(this.stop$));
180178
},
181179

182180
get(id: string) {
183-
const link = navLinks$.value.get(id);
181+
const link = allNavLinks$.value.get(id);
184182
return link && link.properties;
185183
},
186184

187185
getAll() {
188-
return sortNavLinks(navLinks$.value);
186+
return sortLinks(allNavLinks$.value);
189187
},
190188

191189
has(id: string) {
192-
return navLinks$.value.has(id);
190+
return allNavLinks$.value.has(id);
193191
},
194192

195193
showOnly(id: string) {
@@ -237,16 +235,9 @@ export class NavLinksService {
237235
}
238236
}
239237

240-
function sortNavLinks(navLinks: ReadonlyMap<string, NavLinkWrapper>) {
241-
return sortBy(
242-
[...navLinks.values()].map((link) => link.properties),
243-
'order'
244-
);
245-
}
246-
247-
function sortChromeNavLinks(chromeNavLinks: ReadonlyMap<string, ChromeNavLink>) {
238+
function sortLinks(links: ReadonlyMap<string, NavLinkWrapper | ChromeNavLink>) {
248239
return sortBy(
249-
[...chromeNavLinks.values()].map((link) => link as Readonly<ChromeNavLink>),
240+
[...links.values()].map((link) => ('properties' in link ? link.properties : link)),
250241
'order'
251242
);
252243
}

0 commit comments

Comments
 (0)