Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
74 changes: 41 additions & 33 deletions projects/common/src/time/time-range.service.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
import { Injectable } from '@angular/core';
import { ParamMap } from '@angular/router';
import { isEmpty, isNil } from 'lodash-es';
import { EMPTY, Observable, ReplaySubject } from 'rxjs';
import { isEmpty, isNil, omit } from 'lodash-es';
import { concat, EMPTY, Observable, ReplaySubject } from 'rxjs';
import { catchError, distinctUntilChanged, 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 @@ -27,8 +24,7 @@ export class TimeRangeService {

public constructor(
private readonly navigationService: NavigationService,
private readonly timeDurationService: TimeDurationService,
private readonly featureStateResolver: FeatureStateResolver
private readonly timeDurationService: TimeDurationService
) {
this.initializeTimeRange();
this.navigationService.registerGlobalQueryParamKey(TimeRangeService.TIME_RANGE_QUERY_PARAM);
Expand Down Expand Up @@ -57,15 +53,18 @@ export class TimeRangeService {
}

public setRelativeRange(value: number, unit: TimeUnit): this {
return this.setTimeRange(TimeRangeService.toRelativeTimeRange(value, unit));
// return this.setTimeRange(TimeRangeService.toRelativeTimeRange(value, unit));
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed thanks, noticed just have i pushed

return this.setTimeRangeInUrl(TimeRangeService.toRelativeTimeRange(value, unit));
}

public setFixedRange(startTime: Date, endTime: Date): this {
return this.setTimeRange(TimeRangeService.toFixedTimeRange(startTime, endTime));
// return this.setTimeRange(TimeRangeService.toFixedTimeRange(startTime, endTime));
return this.setTimeRangeInUrl(TimeRangeService.toFixedTimeRange(startTime, endTime));
}

public refresh(): void {
this.setTimeRange(this.getCurrentTimeRange());
const currentStringTimeRange = this.getCurrentTimeRange().toUrlString();
this.applyTimeRangeChange(this.timeRangeFromUrlString(currentStringTimeRange));
}

private getInitialTimeRange(): Observable<TimeRange> {
Expand All @@ -82,7 +81,7 @@ export class TimeRangeService {

private getPageTimeRangeChanges(): Observable<TimeRange> {
return this.navigationService.navigation$.pipe(
switchMap(activeRoute => activeRoute.queryParamMap),
map(activeRoute => activeRoute.snapshot.queryParamMap),
filter(queryParamMap => !isNil(queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))),
distinctUntilChanged((prevParamMap, currParamMap) => this.shouldNotUpdateTimeRange(prevParamMap, currParamMap)),
map(currQueryParamMap => {
Expand All @@ -93,6 +92,21 @@ export class TimeRangeService {
);
}

private initializeTimeRange(): void {
concat(this.getInitialTimeRange(), this.getPageTimeRangeChanges()).subscribe(timeRange => {
Copy link
Contributor

Choose a reason for hiding this comment

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

one option discussed offline - the two branches of the subscribe basically handle the respective emissions of these two observables. We could do it as two separate subscriptions and it would probably be easier to understand.

if (!this.timeRangeMatchesCurrentUrl(timeRange)) {
this.setTimeRangeInUrl(timeRange);
} else {
this.applyTimeRangeChange(timeRange);

const queryParams = this.navigationService.getCurrentActivatedRoute().snapshot.queryParams;
if (TimeRangeService.REFRESH_ON_NAVIGATION in queryParams) {
this.navigationService.replaceQueryParametersInUrl(omit(queryParams, TimeRangeService.REFRESH_ON_NAVIGATION));
}
}
});
}

private shouldNotUpdateTimeRange(prevParamMap: ParamMap, currParamMap: ParamMap): boolean {
const refreshQueryParam = currParamMap.get(TimeRangeService.REFRESH_ON_NAVIGATION);

Expand All @@ -102,23 +116,6 @@ export class TimeRangeService {
);
}

private initializeTimeRange(): void {
this.featureStateResolver
.getFeatureState(ApplicationFeature.PageTimeRange)
.pipe(
switchMap(featureState => {
if (featureState === FeatureState.Enabled) {
return this.getPageTimeRangeChanges();
}

return this.getInitialTimeRange();
})
)
.subscribe(timeRange => {
this.setTimeRange(timeRange);
});
}

public timeRangeFromUrlString(timeRangeFromUrl: string): TimeRange {
const duration = this.timeDurationService.durationFromString(timeRangeFromUrl);
if (duration) {
Expand All @@ -132,19 +129,24 @@ export class TimeRangeService {
throw new Error(); // Caught in observable
}

private setTimeRange(newTimeRange: TimeRange): this {
private applyTimeRangeChange(newTimeRange: TimeRange): this {
this.currentTimeRange = newTimeRange;
this.timeRangeSubject$.next(newTimeRange);

return this;
}

private setTimeRangeInUrl(timeRange: TimeRange): this {
this.navigationService.addQueryParametersToUrl({
[TimeRangeService.TIME_RANGE_QUERY_PARAM]: newTimeRange.toUrlString()
[TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString()
});

return this;
}

public setDefaultTimeRange(timeRange: TimeRange): void {
if (!this.currentTimeRange) {
this.setTimeRange(timeRange);
this.setTimeRangeInUrl(timeRange);
}
}

Expand All @@ -157,17 +159,23 @@ export class TimeRangeService {
}

public toQueryParams(timeRange: TimeRange, refreshTimeOnNavigationParam?: boolean): QueryParamObject {
let queryParams: QueryParamObject = {
const queryParams: QueryParamObject = {
[TimeRangeService.TIME_RANGE_QUERY_PARAM]: timeRange.toUrlString()
};

if (refreshTimeOnNavigationParam) {
queryParams = { ...queryParams, [TimeRangeService.REFRESH_ON_NAVIGATION]: true };
return { ...queryParams, [TimeRangeService.REFRESH_ON_NAVIGATION]: true };
}

return queryParams;
}

private timeRangeMatchesCurrentUrl(timeRange: TimeRange): boolean {
return (
this.navigationService.getQueryParameter(TimeRangeService.TIME_RANGE_QUERY_PARAM, '') === timeRange.toUrlString()
);
}

public isInitialized(): boolean {
return !isNil(this.currentTimeRange);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { NavItemLinkConfig, NavViewStyle } from '../navigation.config';
styleUrls: ['./nav-item.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<ht-link *ngIf="this.config" [paramsOrUrl]="buildNavigationParam | htMemoize: this.config">
<ht-link *ngIf="this.config" [paramsOrUrl]="this.buildNavigationParam(this.config)">
<div
*htIfFeature="this.config.featureState$ | async as featureState"
class="nav-item"
Expand Down Expand Up @@ -57,28 +57,23 @@ export class NavItemComponent {
@Input()
public readonly navItemViewStyle?: NavViewStyle;

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

if (this.config.pageLevelTimeRangeIsEnabled && this.config.timeRangeResolver) {
const timeRange = this.config.timeRangeResolver();
const timeRangeQueryParam = this.timeRangeService.toQueryParams(timeRange, true);

navParams = {
return {
...navParams,
queryParams: {
...timeRangeQueryParam
}
queryParams: this.timeRangeService.toQueryParams(this.config.timeRangeResolver(), true)
};
}

return navParams;
};
}

public constructor(
private readonly activatedRoute: ActivatedRoute,
Expand Down
8 changes: 4 additions & 4 deletions projects/components/src/time-range/time-range.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ export class TimeRangeComponent {
text$: of('Refresh'),
role: ButtonRole.Tertiary,
isEmphasized: false,
onClick: () => this.onRefresh(timeRange)
onClick: () => this.onRefresh()
}),
this.ngZone.runOutsideAngular(() =>
// Long running timer will prevent zone from stabilizing
Expand All @@ -143,7 +143,7 @@ export class TimeRangeComponent {
),
role: ButtonRole.Primary,
isEmphasized: true,
onClick: () => this.onRefresh(timeRange)
onClick: () => this.onRefresh()
}))
)
)
Expand All @@ -153,8 +153,8 @@ export class TimeRangeComponent {
return EMPTY;
};

private onRefresh(timeRange: RelativeTimeRange): void {
this.timeRangeService.setRelativeRange(timeRange.duration.value, timeRange.duration.unit);
private onRefresh(): void {
this.timeRangeService.refresh();
}
}

Expand Down
11 changes: 8 additions & 3 deletions src/app/shared/feature-resolver/feature-resolver.service.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import { Injectable } from '@angular/core';
import { FeatureState, FeatureStateResolver } from '@hypertrace/common';
import { ApplicationFeature, FeatureState, FeatureStateResolver } from '@hypertrace/common';
import { Observable, of } from 'rxjs';

@Injectable()
export class FeatureResolverService extends FeatureStateResolver {
public getFeatureState(_: string): Observable<FeatureState> {
return of(FeatureState.Enabled);
public getFeatureState(flag: string): Observable<FeatureState> {
switch (flag) {
case ApplicationFeature.PageTimeRange:
return of(FeatureState.Disabled);
default:
return of(FeatureState.Enabled);
}
}
}