Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
33 changes: 25 additions & 8 deletions projects/common/src/time/time-range.service.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Injectable } from '@angular/core';
import { isEmpty, isNil } from 'lodash-es';
import { EMPTY, Observable, ReplaySubject } from 'rxjs';
import { catchError, filter, map, switchMap, take } from 'rxjs/operators';
import { catchError, filter, map, pairwise, 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';
Expand All @@ -19,6 +19,7 @@ import { TimeUnit } from './time-unit.type';
})
export class TimeRangeService {
private static readonly TIME_RANGE_QUERY_PARAM: string = 'time';
private static readonly REFRESH_ON_NAVIGATION: string = 'refresh';

private readonly timeRangeSubject$: ReplaySubject<TimeRange> = new ReplaySubject(1);
private currentTimeRange?: TimeRange;
Expand Down Expand Up @@ -66,7 +67,7 @@ export class TimeRangeService {
this.setTimeRange(this.getCurrentTimeRange());
}

private globalTimeRangeInitConfigAndChanges(): Observable<TimeRange> {
private getInitialTimeRange(): Observable<TimeRange> {
return this.navigationService.navigation$.pipe(
take(1), // Wait for first navigation
switchMap(activatedRoute => activatedRoute.queryParamMap), // Get the params from it
Expand All @@ -78,12 +79,22 @@ export class TimeRangeService {
);
}

private pageTimeRangeInitConfigAndChanges(): Observable<TimeRange> {
private getPageTimeRangeChanges(): Observable<TimeRange> {
return this.navigationService.navigation$.pipe(
switchMap(activeRoute => activeRoute.queryParamMap),
filter(queryParmaMap => !isNil(queryParmaMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))),
map(queryParmaMap => {
const timeRangeQueryParamString = queryParmaMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM);
filter(queryParamMap => !isNil(queryParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM))),
pairwise(),
filter(([prevParamMap, currParamMap]) => {
const refreshQueryParam = currParamMap.get(TimeRangeService.REFRESH_ON_NAVIGATION);

return (
prevParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM) !==
currParamMap.get(TimeRangeService.TIME_RANGE_QUERY_PARAM) ||
(isEmpty(refreshQueryParam) ? false : JSON.parse(refreshQueryParam!))
);
}),
map(([, currQueryParamMap]) => {
const timeRangeQueryParamString = currQueryParamMap.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.

})
Expand All @@ -96,10 +107,10 @@ export class TimeRangeService {
.pipe(
switchMap(featureState => {
if (featureState === FeatureState.Enabled) {
return this.pageTimeRangeInitConfigAndChanges();
return this.getPageTimeRangeChanges();
}

return this.globalTimeRangeInitConfigAndChanges();
return this.getInitialTimeRange();
})
)
.subscribe(timeRange => {
Expand Down Expand Up @@ -150,6 +161,12 @@ export class TimeRangeService {
};
}

public getRefreshTimeRangeQueryParam(): QueryParamObject {
return {
[TimeRangeService.REFRESH_ON_NAVIGATION]: true
};
}

public isInitialized(): boolean {
return !isNil(this.currentTimeRange);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,15 @@ export class NavItemComponent {

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

navParams = {
...navParams,
queryParams: this.timeRangeService.toQueryParams(timeRange)
queryParams: {
...timeRangeQueryParam,
...timeRangeRefreshQueryParam
}
};
}

Expand Down