Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1de4d4d
refactor: re-structure of page time range to be query param centric
Christian862 Mar 29, 2022
9f10747
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into NavIt…
Christian862 Mar 29, 2022
90aca7d
test: support for recent changes
Christian862 Mar 29, 2022
40f55da
refactor: requested changes
Christian862 Mar 29, 2022
31d4f0f
refactor: added refresh query param
Christian862 Mar 30, 2022
bdb7a8b
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into NavIt…
Christian862 Mar 30, 2022
595017a
refactor: name change
Christian862 Mar 30, 2022
e70957d
refactor: requested changes
Christian862 Mar 30, 2022
898dc90
refactor: option param type for refresh flag
Christian862 Mar 30, 2022
0c88198
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into NavIt…
Christian862 Mar 30, 2022
be3d30f
refactor: testing support
Christian862 Mar 30, 2022
7f4a4f1
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into NavIt…
Christian862 Mar 30, 2022
8a9660e
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into NavIt…
Christian862 Mar 31, 2022
17fa1b3
fix: nit requested changes
Christian862 Mar 31, 2022
c03ed28
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into NavIt…
Christian862 Apr 1, 2022
9680f55
refactor: aarons changes
Christian862 Apr 1, 2022
75f5be9
refactor: requested changes
Christian862 Apr 1, 2022
c8f632e
refactor: remove htMemoize from nav item
Christian862 Apr 1, 2022
a2e33fc
test: added and fixed testing for new time range
Christian862 Apr 2, 2022
e542a30
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into NavIt…
Christian862 Apr 4, 2022
1321830
refactor: restore momoization for nav item
Christian862 Apr 4, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions projects/common/src/navigation/ht-route-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ export interface HtRouteData {
features?: string[];
title?: string;
defaultTimeRange?: TimeRange;
shouldSavePageTimeRange?: boolean;
}
14 changes: 8 additions & 6 deletions projects/common/src/time/page-time-range-preference.service.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Injectable } from '@angular/core';
import { ActivatedRoute } from '@angular/router';
import { isNil } from 'lodash-es';
import { combineLatest, Observable } from 'rxjs';
import { map, shareReplay, take } from 'rxjs/operators';
Expand Down Expand Up @@ -38,7 +37,7 @@ export class PageTimeRangePreferenceService {
map(([pageTimeRangeStringDictionary, featureState]) => {
if (featureState === FeatureState.Enabled) {
if (isNil(pageTimeRangeStringDictionary[rootLevelPath])) {
return () => this.getDefaultTimeRangeForCurrentRoute();
return () => this.getDefaultTimeRangeForCurrentRoute(rootLevelPath);
}

return () => this.timeRangeService.timeRangeFromUrlString(pageTimeRangeStringDictionary[rootLevelPath]);
Expand Down Expand Up @@ -78,11 +77,14 @@ export class PageTimeRangePreferenceService {
.pipe(shareReplay(1));
}

public getDefaultTimeRangeForCurrentRoute(): TimeRange {
const currentRoute: ActivatedRoute = this.navigationService.getCurrentActivatedRoute();
// Right side for when FF is enabled but 'defaultTimeRange' is not set on AR data
public getDefaultTimeRangeForCurrentRoute(rootLevelPath: string): TimeRange {
const routeConfigForPath = this.navigationService.getRouteConfig(
[rootLevelPath],
this.navigationService.rootRoute()
);

return currentRoute.snapshot.data?.defaultTimeRange ?? this.getGlobalDefaultTimeRange();
// Right side for when FF is enabled but 'defaultTimeRange' is not set on AR data
return routeConfigForPath?.data?.defaultTimeRange ?? this.getGlobalDefaultTimeRange();
}

public getGlobalDefaultTimeRange(): TimeRange {
Expand Down
63 changes: 50 additions & 13 deletions projects/common/src/time/time-range.service.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { Injectable } from '@angular/core';
import { isEmpty, isNil } from 'lodash-es';
import { EMPTY, ReplaySubject } from 'rxjs';
import { EMPTY, Observable, ReplaySubject } from 'rxjs';
import { catchError, filter, map, switchMap, take } from 'rxjs/operators';
import { ApplicationFeature } from '../constants/application-constants';
import { FeatureStateResolver } from '../feature/state/feature-state.resolver';
import { FeatureState } from '../feature/state/feature.state';
import { NavigationService, QueryParamObject } from '../navigation/navigation.service';
import { ReplayObservable } from '../utilities/rxjs/rxjs-utils';
import { FixedTimeRange } from './fixed-time-range';
Expand All @@ -22,7 +25,8 @@ export class TimeRangeService {

public constructor(
private readonly navigationService: NavigationService,
private readonly timeDurationService: TimeDurationService
private readonly timeDurationService: TimeDurationService,
private readonly featureStateResolver: FeatureStateResolver
) {
this.initializeTimeRange();
this.navigationService.registerGlobalQueryParamKey(TimeRangeService.TIME_RANGE_QUERY_PARAM);
Expand Down Expand Up @@ -62,16 +66,44 @@ export class TimeRangeService {
this.setTimeRange(this.getCurrentTimeRange());
}

private globalTimeRangeInitialization(): Observable<TimeRange> {
return this.navigationService.navigation$.pipe(
take(1), // Wait for first navigation
switchMap(activatedRoute => activatedRoute.queryParamMap), // Get the params from it
take(1), // Only the first set of params
map(paramMap => paramMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM)), // Extract the time range value from it
filter((timeRangeString): timeRangeString is string => !isEmpty(timeRangeString)), // Only valid time ranges
map(timeRangeString => this.timeRangeFromUrlString(timeRangeString)),
catchError(() => EMPTY)
);
}

private pageTimeRangeInitialization(): Observable<TimeRange> {
return this.navigationService.navigation$.pipe(
filter(
activatedRoute => !isNil(activatedRoute.snapshot.queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))
),
map(activeRoute => {
const timeRangeQueryParamString = activeRoute.snapshot.queryParamMap.get(
TimeRangeService.TIME_RANGE_QUERY_PARAM
);

return this.timeRangeFromUrlString(timeRangeQueryParamString!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when do we want to emit?

  • Current code here would change the time range every time we navigate. That would break flows like drilling down in the same TR
  • We could do it only when the TR string changes (e.g. 1h -> 1d), but that means if we change top level items when the setting is the same, we wouldn't refresh (this is the behavior today, so seems acceptable)
  • If we need to know when to refresh and when not to, we'll likely need another param

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this only emits when we navigate and have a time query param. Drill behaviour still works as expected.

})
);
}

private initializeTimeRange(): void {
this.navigationService.navigation$
this.featureStateResolver
.getFeatureState(ApplicationFeature.PageTimeRange)
.pipe(
take(1), // Wait for first navigation
switchMap(activatedRoute => activatedRoute.queryParamMap), // Get the params from it
take(1), // Only the first set of params
map(paramMap => paramMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM)), // Extract the time range value from it
filter((timeRangeString): timeRangeString is string => !isEmpty(timeRangeString)), // Only valid time ranges
map(timeRangeString => this.timeRangeFromUrlString(timeRangeString)),
catchError(() => EMPTY)
switchMap(featureState => {
if (featureState === FeatureState.Enabled) {
return this.pageTimeRangeInitialization();
}

return this.globalTimeRangeInitialization();
})
)
.subscribe(timeRange => {
this.setTimeRange(timeRange);
Expand Down Expand Up @@ -115,11 +147,16 @@ export class TimeRangeService {
return new FixedTimeRange(startTime, endTime);
}

public toQueryParams(startTime: Date, endTime: Date): QueryParamObject {
const newTimeRange = new FixedTimeRange(startTime, endTime);
public toQueryParams(startTime: Date, endTime: Date, timeRangeIfRelative?: TimeRange): QueryParamObject {
if (timeRangeIfRelative && timeRangeIfRelative instanceof RelativeTimeRange) {
return {
[TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRangeIfRelative.toUrlString()
};
}
const newFixedTimeRange = new FixedTimeRange(startTime, endTime);

return {
[TimeRangeService.TIME_RANGE_QUERY_PARAM]: newTimeRange.toUrlString()
[TimeRangeService.TIME_RANGE_QUERY_PARAM]: newFixedTimeRange.toUrlString()
};
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ChangeDetectionStrategy, Component, Input } from '@angular/core';
import { ActivatedRoute } from '@angular/router';
import { FeatureState, NavigationParams, NavigationParamsType } from '@hypertrace/common';
import { FeatureState, NavigationParams, NavigationParamsType, TimeRangeService } from '@hypertrace/common';
import { IconSize } from '../../icon/icon-size';
import { NavItemLinkConfig, NavViewStyle } from '../navigation.config';

Expand Down Expand Up @@ -57,12 +57,23 @@ export class NavItemComponent {
@Input()
public readonly navItemViewStyle?: NavViewStyle;

public buildNavigationParam = (item: NavItemLinkConfig): NavigationParams => ({
navType: NavigationParamsType.InApp,
path: item.matchPaths[0],
relativeTo: this.activatedRoute,
replaceCurrentHistory: item.replaceCurrentHistory
});
public buildNavigationParam = (item: NavItemLinkConfig): NavigationParams => {
const timeRange = this.config.timeRangeResolver ? this.config.timeRangeResolver() : undefined;

public constructor(private readonly activatedRoute: ActivatedRoute) {}
return {
navType: NavigationParamsType.InApp,
path: item.matchPaths[0],
relativeTo: this.activatedRoute,
replaceCurrentHistory: item.replaceCurrentHistory,
queryParams:
timeRange && this.config.pageLevelTimeRangeIsEnabled
? this.timeRangeService.toQueryParams(timeRange.startTime, timeRange.endTime, timeRange)
: undefined
};
};

public constructor(
private readonly activatedRoute: ActivatedRoute,
private readonly timeRangeService: TimeRangeService
) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import {
<ng-container *ngSwitchCase="'${NavItemType.Link}'">
<ht-nav-item
[navItemViewStyle]="this.navViewStyle"
(click)="this.navItemClick.emit(item)"
[config]="item"
[active]="item === activeItem"
[collapsed]="this.collapsed"
Expand Down Expand Up @@ -95,9 +94,6 @@ export class NavigationListComponent implements OnChanges {
@Input()
public readonly navViewStyle?: NavViewStyle;

@Output()
public readonly navItemClick: EventEmitter<NavItemLinkConfig> = new EventEmitter();

@Output()
public readonly activeItemChange: EventEmitter<NavItemLinkConfig> = new EventEmitter();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export class PageTimeRangeComponent {
}

public shouldSavePageTimeRange(currentRoute: ActivatedRoute): boolean {
return !isNil(currentRoute.snapshot.data?.defaultTimeRange);
return !isNil(currentRoute.snapshot.data?.shouldSavePageTimeRange);
}

public savePageTimeRange(selectedTimeRange: TimeRange, segment: UrlSegment): void {
Expand Down
18 changes: 12 additions & 6 deletions src/app/routes/root-routing.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ const ROUTE_CONFIG: HtRoute[] = [
icon: IconType.Dashboard,
label: 'Dashboard'
},
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour))
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)),
shouldSavePageTimeRange: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could infer this - up to you. There's some scenarios where we want to specify a route that should save but doesn't define its own time range - if it does define it's own though, we'd always want to save right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this crossed my mind. Correct, if the route defines it's own, we'd always want to save. However I'm gonna leave it as is for readability - to anyone looking at these route configs without the full picture, I think it'd be easier to understand with this property here. One condition being satisfied based on one property rather than two, is a bit clearer

},
loadChildren: () => import('../home/home.module').then(module => module.HomeModule)
},
Expand All @@ -37,7 +38,8 @@ const ROUTE_CONFIG: HtRoute[] = [
icon: ObservabilityIconType.ApplicationFlow,
label: 'Application Flow'
},
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour))
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)),
shouldSavePageTimeRange: true
},
loadChildren: () =>
import('./application-flow/application-flow-routing.module').then(
Expand All @@ -51,7 +53,8 @@ const ROUTE_CONFIG: HtRoute[] = [
icon: ObservabilityIconType.Backend,
label: 'Backends'
},
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour))
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)),
shouldSavePageTimeRange: true
},
loadChildren: () =>
import('./backends/backends-routing.module').then(module => module.BackendsRoutingModule)
Expand All @@ -63,7 +66,8 @@ const ROUTE_CONFIG: HtRoute[] = [
icon: ObservabilityIconType.Service,
label: 'Services'
},
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour))
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)),
shouldSavePageTimeRange: true
},
loadChildren: () =>
import('./services/services-routing.module').then(module => module.ServicesRoutingModule)
Expand All @@ -75,7 +79,8 @@ const ROUTE_CONFIG: HtRoute[] = [
icon: ObservabilityIconType.Api,
label: 'Endpoints'
},
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour))
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)),
shouldSavePageTimeRange: true
},
loadChildren: () =>
import('./endpoints/endpoint-routing.module').then(module => module.EndpointRoutingModule)
Expand All @@ -95,7 +100,8 @@ const ROUTE_CONFIG: HtRoute[] = [
icon: IconType.Search,
label: 'Explorer'
},
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour))
defaultTimeRange: new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)),
shouldSavePageTimeRange: true
},
loadChildren: () =>
import('./explorer/explorer-routing.module').then(module => module.ExplorerRoutingModule)
Expand Down
13 changes: 2 additions & 11 deletions src/app/shared/navigation/navigation.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ import { Observable } from 'rxjs';
*htLetAsync="this.isCollapsed$ as isCollapsed"
[collapsed]="isCollapsed"
(collapsedChange)="this.onViewToggle($event)"
(navItemClick)="this.setPageTimeRangeForSelectedNavItem($event)"
(activeItemChange)="this.updateDefaultTimeRangeIfUnset($event)"
(activeItemChange)="this.setPageTimeRangeForActiveNavItem($event)"
></ht-navigation-list>
</div>
`
Expand Down Expand Up @@ -101,7 +100,7 @@ export class NavigationComponent {
this.isCollapsed$ = this.preferenceService.get(NavigationComponent.COLLAPSED_PREFERENCE, false);
}

public setPageTimeRangeForSelectedNavItem(navItemLink: NavItemLinkConfig): void {
public setPageTimeRangeForActiveNavItem(navItemLink: NavItemLinkConfig): void {
if (!isNil(navItemLink.timeRangeResolver) && navItemLink.pageLevelTimeRangeIsEnabled) {
const timeRange = navItemLink.timeRangeResolver();
if (timeRange instanceof FixedTimeRange) {
Expand All @@ -112,14 +111,6 @@ export class NavigationComponent {
}
}

public updateDefaultTimeRangeIfUnset(activeItem: NavItemLinkConfig): void {
// Initialize the time range service
// Depending on FF status, the TR will be either global or page level for the init
if (!this.timeRangeService.isInitialized()) {
this.timeRangeService.setDefaultTimeRange(activeItem.timeRangeResolver!());
}
}

public onViewToggle(collapsed: boolean): void {
this.preferenceService.set(NavigationComponent.COLLAPSED_PREFERENCE, collapsed);
}
Expand Down