-
Notifications
You must be signed in to change notification settings - Fork 11
Page level time range #1441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Page level time range #1441
Changes from 18 commits
Commits
Show all changes
63 commits
Select commit
Hold shift + click to select a range
dd20f13
refactor: time range styles
a8523ab
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
4bfa6d6
feat: initial browser stored page time range
63a665e
fix: removed defaultTimeRange null check for throwing error
b81ae1a
refactor: time range logic to be query param centric
6e84fbe
refactor: update effected tests
ab466fe
Merge branch 'main' into BreadcrumbsTimeRange
3043f45
fix: replace arrow function for compiler
56c574b
fix: time range isCustom correction
c1be553
refactor: moved time range logic to sub component
8eb3a52
fix: adjusted time range style to match other header buttons
ffe3d6b
fix: linting
3f98a99
refactor: cleaned up nav params observable
f0d9ffe
fix: linting
e1bea69
refactor: feature flag added
b359fcd
test: tests for new time range
ab36992
test: linter
cd870bc
Merge branch 'main' into BreadcrumbsTimeRange
7a92d50
test: added new test for time range service
b241e0e
fix: pr changes
7e279e4
fix: linting
10e8225
Merge branch 'main' into BreadcrumbsTimeRange
1738b10
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
c44b03b
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
bd1cbbd
Merge branch 'BreadcrumbsTimeRange' of github.com:hypertrace/hypertra…
3adc661
fix: requested changes
6e78e54
fix: export feature enum
c225cf9
refactor: replaced time range icon with calendar
c2d0287
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
7bc71c7
fix: requested changes
b4b4722
fix: backwards compatability and depency issue changes
a528987
fix: requested changes
63c4bd0
fix: requested changes
8742a2b
fix: requested changes
b795d39
fix: linting
9ed2b22
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
67c8bfe
fix: moved tr selector back to components
0854000
fix: update test
62d1041
refactor: requested changes
5037200
refactor: requested changes
Christian862 11b0991
refactor: requested changes
Christian862 50153f9
refactor: requested changes
Christian862 12ab84d
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
Christian862 ea6357a
refactor: requested changes
Christian862 b558d41
fix: linting
Christian862 0989a6a
fix: requested changes - naming and TR service
Christian862 3845357
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
Christian862 a37a477
fix: requested changes
Christian862 00f785a
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
Christian862 82a9612
fix: requested changes - broken
Christian862 9714df8
fix: requested changes
Christian862 4b0b74e
test: testing support for new TR init
Christian862 44a67b6
refactor: requested changes
Christian862 bfcb9b8
refactor: requested changes, updated route data for new changes
Christian862 38c5e82
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
Christian862 efd4d15
refactor: requested changes
Christian862 5e5dcf5
refactor: requested changes
Christian862 7172091
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
Christian862 505d5d7
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
Christian862 748743e
refactor: requested changes
Christian862 09cf639
refactor: requested changes
Christian862 5c0cfe6
refactor: fix FF string
Christian862 541555f
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Bread…
Christian862 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| import { Observable } from 'rxjs'; | ||
| import { TimeRange } from '../time/time-range'; | ||
| import { Breadcrumb } from './breadcrumb'; | ||
|
|
||
| export interface HtRouteData { | ||
| breadcrumb?: Breadcrumb | Observable<Breadcrumb>; | ||
| features?: string[]; | ||
| title?: string; | ||
| defaultTimeRange?: TimeRange; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,6 @@ export class FixedTimeRange implements TimeRange { | |
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed with @anandtiwary, the values here and in RelativeTimeRange were a mistake and assigned incorrectly. Fixing them here because part of the PR depends on them. Other than this PR no other places use them |
||
| public isCustom(): boolean { | ||
| // Right now all RelativeTimeRanges are NOT custom; all FixedTimeRanges are | ||
| return false; | ||
| return true; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { | ||
| FixedTimeRange, | ||
| NavigationService, | ||
| PageTimeRangeService, | ||
| RelativeTimeRange, | ||
| TimeDuration, | ||
| TimeRange, | ||
| TimeRangeService, | ||
| TimeUnit | ||
| } from '@hypertrace/common'; | ||
| import { runFakeRxjs } from '@hypertrace/test-utils'; | ||
| import { createServiceFactory, mockProvider } from '@ngneat/spectator/jest'; | ||
|
|
||
| describe('Page time range service', () => { | ||
| const serviceFactory = createServiceFactory({ | ||
| service: PageTimeRangeService, | ||
| providers: [mockProvider(NavigationService)] | ||
| }); | ||
|
|
||
| test('Setting fixed time range emits corresponding time range from preferences', () => { | ||
| runFakeRxjs(({ expectObservable, cold }) => { | ||
| const timeRange: TimeRange = new FixedTimeRange(new Date(1573255100253), new Date(1573255111159)); | ||
| const spectator = serviceFactory({ | ||
| providers: [ | ||
| mockProvider(TimeRangeService, { | ||
| timeRangeFromUrlString: jest.fn().mockReturnValue(timeRange) | ||
| }) | ||
| ] | ||
| }); | ||
|
|
||
| cold('-a|', { | ||
| a: () => spectator.service.setPageTimeRange('foo', timeRange) | ||
| }).subscribe(update => update()); | ||
|
|
||
| expectObservable(spectator.service.getPageTimeRange('foo')).toBe('da', { | ||
| d: undefined, | ||
| a: timeRange | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| test('Setting relative time range emits corresponding time range from preferences', () => { | ||
| runFakeRxjs(({ expectObservable, cold }) => { | ||
| const timeRange: TimeRange = new RelativeTimeRange(new TimeDuration(1, TimeUnit.Hour)); | ||
| const spectator = serviceFactory({ | ||
| providers: [ | ||
| mockProvider(TimeRangeService, { | ||
| timeRangeFromUrlString: jest.fn().mockReturnValue(timeRange) | ||
| }) | ||
| ] | ||
| }); | ||
|
|
||
| cold('-b|', { | ||
| b: () => spectator.service.setPageTimeRange('bar', timeRange) | ||
| }).subscribe(update => update()); | ||
|
|
||
| expectObservable(spectator.service.getPageTimeRange('bar')).toBe('db', { | ||
| d: undefined, | ||
| b: timeRange | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { Injectable, OnDestroy } from '@angular/core'; | ||
| import { isNil } from 'lodash-es'; | ||
| import { BehaviorSubject, Observable, Subject } from 'rxjs'; | ||
| import { distinctUntilChanged, map, share, takeUntil } from 'rxjs/operators'; | ||
| import { PreferenceService, StorageType } from '../preference/preference.service'; | ||
| import { TimeRange } from './time-range'; | ||
| import { TimeRangeService } from './time-range.service'; | ||
|
|
||
| @Injectable({ providedIn: 'root' }) | ||
| export class PageTimeRangeService implements OnDestroy { | ||
Christian862 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // TODO change to local ?? | ||
| private static readonly STORAGE_TYPE: StorageType = StorageType.Session; | ||
| private static readonly TIME_RANGE_PREFERENCE_KEY: string = 'page-time-range'; | ||
| private readonly destroyed$: Subject<void> = new Subject(); | ||
|
|
||
| private readonly pageTimeRanges$: BehaviorSubject<PageTimeRangeMap> = new BehaviorSubject<PageTimeRangeMap>({}); | ||
| private readonly storedTimeRanges$: Observable<PageTimeRangeMap> = this.preferenceService | ||
| .get<PageTimeRangeMap>(PageTimeRangeService.TIME_RANGE_PREFERENCE_KEY, {}, PageTimeRangeService.STORAGE_TYPE) | ||
| .pipe(takeUntil(this.destroyed$)); | ||
|
|
||
| public constructor( | ||
| private readonly preferenceService: PreferenceService, | ||
| private readonly timeRangeService: TimeRangeService | ||
| ) { | ||
| this.storedTimeRanges$.subscribe(values => { | ||
| this.pageTimeRanges$.next(values); | ||
| }); | ||
| } | ||
|
|
||
| public getPageTimeRange(path: string): Observable<TimeRange | undefined> { | ||
| return this.storedTimeRanges$.pipe( | ||
| distinctUntilChanged((prev, curr) => prev[path] === curr[path]), | ||
| map(timeRanges => | ||
| !isNil(timeRanges[path]) ? this.timeRangeService.timeRangeFromUrlString(timeRanges[path]) : undefined | ||
| ), | ||
| share() | ||
| ); | ||
| } | ||
|
|
||
| public setPageTimeRange(path: string, value: TimeRange): void { | ||
| const pageTimeMap: PageTimeRangeMap = this.pageTimeRanges$.getValue(); | ||
Christian862 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| const newMap: PageTimeRangeMap = { ...pageTimeMap, [path]: value.toUrlString() }; | ||
|
|
||
| this.preferenceService.set( | ||
| PageTimeRangeService.TIME_RANGE_PREFERENCE_KEY, | ||
| newMap, | ||
| PageTimeRangeService.STORAGE_TYPE | ||
| ); | ||
| } | ||
|
|
||
| public ngOnDestroy(): void { | ||
Christian862 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| this.destroyed$.next(); | ||
| this.destroyed$.complete(); | ||
| } | ||
| } | ||
|
|
||
| interface PageTimeRangeMap { | ||
| [path: string]: string; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 changes: 8 additions & 0 deletions
8
projects/components/src/header/header-content/header-content-primary.directive.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { Directive, TemplateRef } from '@angular/core'; | ||
|
|
||
| @Directive({ | ||
| selector: '[htHeaderContentPrimary]' | ||
| }) | ||
| export class HeaderContentPrimaryDirective { | ||
| public constructor(public templateRef: TemplateRef<unknown>) {} | ||
| } |
8 changes: 8 additions & 0 deletions
8
projects/components/src/header/header-content/header-content-secondary.directive.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { Directive, TemplateRef } from '@angular/core'; | ||
|
|
||
| @Directive({ | ||
| selector: '[htHeaderContentSecondary]' | ||
| }) | ||
| export class HeaderContentSecondaryDirective { | ||
Christian862 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| public constructor(public templateRef: TemplateRef<unknown>) {} | ||
| } | ||
12 changes: 12 additions & 0 deletions
12
projects/components/src/header/header-content/header-content.module.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import { CommonModule } from '@angular/common'; | ||
| import { NgModule } from '@angular/core'; | ||
| import { HeaderContentPrimaryDirective } from './header-content-primary.directive'; | ||
| import { HeaderContentSecondaryDirective } from './header-content-secondary.directive'; | ||
|
|
||
| @NgModule({ | ||
| declarations: [HeaderContentPrimaryDirective, HeaderContentSecondaryDirective], | ||
| exports: [HeaderContentPrimaryDirective, HeaderContentSecondaryDirective], | ||
| imports: [CommonModule], | ||
| providers: [] | ||
Christian862 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }) | ||
| export class HeaderContentModule {} | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.