Page level time range #1441
Page level time range #1441
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1441 +/- ##
==========================================
- Coverage 83.98% 83.90% -0.08%
==========================================
Files 862 865 +3
Lines 18358 18471 +113
Branches 2485 2499 +14
==========================================
+ Hits 15418 15499 +81
- Misses 2823 2852 +29
- Partials 117 120 +3
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
projects/components/src/navigation/nav-item/nav-item.component.ts
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
| this.pageTimeRangeStringDictionary$, | ||
| this.featureStateResolver.getCombinedFeatureState([ | ||
| ApplicationFeature.PageTimeRange, | ||
| ApplicationFeature.NavigationRedesign |
There was a problem hiding this comment.
Why is the nav redesign relevant here? The page time range I thought was independent of nav changes.
There was a problem hiding this comment.
PageTimeRange is responsible for bringing the time range down to the the page level, on the headers, however it still uses the original time range component to maintain original time range behaviour . The NavigationRedesign feature enables the time range to behave with page level functionality. This came after talks with @anandtiwary, and i believe this feature dependency structure is why he suggested to create something like this on the backend.
We could just use NavigationRedesign in this instance, however in the case that we disabled page time range and enable NavigationRedesign the ui will be broken because
- The top nav bar won't be present (in the coming nav2.0 PR) so no TR component will exist.
- The nav items will be attempting to use page level time range logic when they shouldn't be(changing TR on click).
There was a problem hiding this comment.
The NavigationRedesign feature enables the time range to behave with page level functionality
Oh interesting, so this is not what I would expect from the naming. I would have expected the behavior changes (the time range is scoped to the page) to come from the PageTimeRange flag, as well as the relocation of the control at the same time - basically all the work in this PR. I thought the Navigation redesign was dependent on that (since the new structure has no time range selector), but otherwise unrelated.
So first off, if we really want that separation, it seems like there's a flag rename in order (but let's see).
We could just use NavigationRedesign in this instance, however in the case that we disabled page time range and enable NavigationRedesign the ui will be broken because
We shouldn't try to add complexity to allow any matrix of flags to work. We can assume dependencies if it saves us significant work, I think that was mentioned earlier. In that scenario you described though, how are we solving the lack of TR? If we've removed top and disabled showing it on the page too, it's just not a valid state. And In that same scenario, given the time range page behavior is part of the Nav redesign flag as you described, we would actually want 2.
|
|
||
| return defaultTimeRange; | ||
| private getDefaultPageTimeRange(): TimeRange | undefined { | ||
| return this.navigationService.getCurrentActivatedRoute().snapshot.data?.defaultTimeRange; |
There was a problem hiding this comment.
Just trying to understand how this works - if I click on A on the left nav, and it routes be to A/B, we need the time range to be defined on B right? But in the dictionary, if I changed it, it would be stored under A? If so, I think that's confusing. If we think A is the important bit there, then the time range being defined there would make sense too, so if I change the default routing it doesn't touch the time range. To do that, you might need to change this to walk up the tree looking.
There was a problem hiding this comment.
Yes that is how it works. @anandtiwary and I briefly looked into traversing the routes to find the correct activated route but it's a little tricky. Because all the navItem time ranges are resolved on initial load, you can't use ActivatedRoute. That's why before i was using navigationService.getRouteConfig - however, this method does not traverse lazy loaded child routes, so we can't use it here either.
One solution would be to keep it as-is with the A/B functionality you described above, and to create a future work item out of this. Another option is to change the timing of when the navItems are decorated to being on click, but that might be a timely effort, requiring a rework in the nav-list component.
There was a problem hiding this comment.
looked into traversing the routes to find the correct activated route but it's a little tricky
We do have this tree walk in a number of places today. For example the feature guard I believe starts from the activated route, and walks up the tree checking the config for each parent route.
Because all the navItem time ranges are resolved on initial load, you can't use ActivatedRoute. That's why before i was using navigationService.getRouteConfig
Not sure why that would differ though - the activated route just gives you another way to get to the route config - either way it's resolved at load time, which is a very good point. We need to rebuild relative time ranges at the point in time where they take effect (will take a look back at the PR with that in mind).
One solution would be to keep it as-is with the A/B functionality you described above, and to create a future work item out of this. Another option is to change the timing of when the navItems are decorated to being on click, but that might be a timely effort, requiring a rework in the nav-list component.
Don't think the latter is either worth it or would work. I think I would go for putting the time range on the root route's config, and changing the lookup to walk up the tree like we do in FF, but up to you if you feel it makes sense to do that now or later.
projects/components/src/navigation/navigation-list-component.service.ts
Outdated
Show resolved
Hide resolved
| // Decorate the nav items with the corresponding time ranges, depending on the FF state. | ||
| // The time ranges in nav items are streams that get the most recent value from page time range preference service | ||
| this.navItems$ = this.featureStateResolver | ||
| .getCombinedFeatureState([ApplicationFeature.PageTimeRange, ApplicationFeature.NavigationRedesign]) |
There was a problem hiding this comment.
It's still unclear to me why these two things are combined. the time range being set per page seems independent of the navigation.
projects/components/src/navigation/navigation-list.component.ts
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
projects/components/src/page-time-range/page-time-range.component.ts
Outdated
Show resolved
Hide resolved
| public resolveNavItemConfigTimeRanges(navItems: NavItemConfig[]): Observable<NavItemConfig[]> { | ||
| return this.featureStateResolver | ||
| .getFeatureState(ApplicationFeature.PageTimeRange) | ||
| .pipe(switchMap(featureState => combineLatest(this.getTimeRangesForNavItems(navItems, featureState)))); |
There was a problem hiding this comment.
So any time a time range is saved, you would get a whole new set of nav items?
There was a problem hiding this comment.
Yes, is there a better operator for this ?
There was a problem hiding this comment.
This gets back to the suggestion to do the resolution on click rather than up front, but we can revisit later as discussed. Just need to make sure with this approach that a new set of items doesn't cause issues (e.g. losing the active item, flickering due to re-render etc.)
projects/components/src/navigation/navigation-list-component.service.ts
Outdated
Show resolved
Hide resolved
| map(() => { | ||
| const activeItem = this.findActiveItem(this.navItems); | ||
| if (!this.timeRangeService.isInitialized() && activeItem?.resolveTimeRange) { | ||
| this.timeRangeService.setDefaultTimeRange(activeItem.resolveTimeRange()); |
There was a problem hiding this comment.
unclear why we're setting this here
There was a problem hiding this comment.
I put the logic for calling setDefault in the activeItem$ pipe because it needs the TR value that is decorated on the navItem, and the knowledge of what nav item is currently active, and this captures both, and also this is a shared component.
There was a problem hiding this comment.
also this is a shared component.
This is my main concern - it's a shared component, and the whole component is used in contexts where we would not want this behavior - I'm guessing that resolveTimeRange is trying to address those cases. I would ideally prefer putting this in the parent left nav instead, unless there's a reason that's not feasible.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
projects/components/src/navigation/navigation-list.component.ts
Outdated
Show resolved
Hide resolved
| if (!this.timeRangeService.isInitialized() && activeItem?.timeRangeResolver) { | ||
| this.timeRangeService.setDefaultTimeRange(activeItem.timeRangeResolver()); | ||
| } | ||
| this.activeItemChange.emit(activeItem); |
There was a problem hiding this comment.
I'm ok with it but would be remiss not to point out - we're using this observable for side effects. If we happened to not read activeItem$ in this component because we didn't need to use it for our own template, it would create a bug where we also stopped emitting.
The alternative however is also not great, since it means we need to subscribe in the component and handle unsubscribing on destroy.
There was a problem hiding this comment.
Yeah fair concern, i've made a code comment about it for anyone that sees this in the future.
| private readonly userTelemetryOrchestrationService: UserTelemetryOrchestrationService, | ||
| private readonly timeRangeService: TimeRangeService | ||
| ) { | ||
| this.timeRangeHasInit$ = this.timeRangeService.getTimeRangeAndChanges(); |
There was a problem hiding this comment.
nit: could take(1) here, will let this clean up rather than hanging around
There was a problem hiding this comment.
Wouldn't that defeat the purpose ? it will take the first emission which will be undefined, and never show the app content
There was a problem hiding this comment.
getTimeRangeAndChanges should never output an undefined - it just won't emit until a time range is assigned. If we broke that, that means it's mistyped and we should go fix.
This comment has been minimized.
This comment has been minimized.
|
|
||
| export const enum ApplicationFeature { | ||
| PageTimeRange = 'ui.page-time-range' | ||
| PageTimeRange = 'ui.page-time-range-' |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Description
This PR covers moving to a page level time range system with left nav page time ranges being stored in the browser session.
Feature flags
This change is rolled out behind two 'nested' feature flags:
PageTimeRangeandNavigationRedesign.PageTimeRangeis responsible for moving the time range component from the application header, onto the page header component. When onlyPageTimeRangeis enabled, the time range is moved to the page headers but maintains original 'global' behaviour, so this is just a component location swap.When
PageTimeRangeandNavigationRedesignare enabled, the behaviour of the time range changes to page level time range ( description below ).General
-If the user is on left nav item page and the time range is selected, it's persisted to browser storage.
defaultTimeRangeproperty in thedataobject of the activated route, will be persisted to browser storage.user-specified-time-range.service.ts), and decorating thenavItemswith the time range corresponding to it's path (decoration:navigation-list.service.ts`).navigation-list.component.tsresponds to this event and uses the decoratednavItemto set the time rangeuser-specified-time-range-selector.component.tsis used to determine whether or not to save the selected time range to storage.Core files to review
There's a good chunk of files in this PR so i'll list here the files with the core logic that should be reviewed more carefully. Additionally i'll comment a small summary at the top of these files.
user-specified-time-range-selector.component.tspage-header.component.tsnavigation-list.component.tsnavigation-list.service.tsnav-item.component.tstime-range.component.tsTesting
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.