Skip to content

Query params resetting on nav item click #1531

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.

Sign up for GitHub

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 5 commits into from
Apr 6, 2022
Merged

Query params resetting on nav item click #1531

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dcdf9c5
fix: environment selector resetting on lef navigation
Christian862 Apr 4, 2022
006961a
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Envir…
Christian862 Apr 5, 2022
bf94ac6
fix: revert previous solution, persist global query params for buildN…
Christian862 Apr 5, 2022
14b69a5
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Envir…
Christian862 Apr 6, 2022
70d85ac
Merge branch 'main' of github.com:hypertrace/hypertrace-ui into Envir…
Christian862 Apr 6, 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
3 changes: 3 additions & 0 deletions projects/components/src/navigation/nav-item/nav-item.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ import { NavItemLinkConfig, NavViewStyle } from '../navigation.config';
`
})
export class NavItemComponent {
private static readonly QUERY_PARAM_HANDLING_STRATEGY = 'merge';
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we considered fixing nav service rather than the particular call? Do we know of use cases in nav service where we'd not want this behavior? seems like a bug to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically I'm referring to changing

        queryParams: params?.queryParams ?? this.buildQueryParam(),
        queryParamsHandling: params?.queryParamsHandling,

to something like

        queryParams: {...this.buildQueryParam(), ...(params?.queryParams ?? {})
        queryParamsHandling: params?.queryParamsHandling,

The idea being that assuming we're doing in app navigation, we never want to remove the global query params that are preserved by the buildQueryParam() call.

Copy link
Contributor

Choose a reason for hiding this comment

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

The more that I think about it, I think the current PR would have more issues. Take the scenario where you navigate away from a page that has its own query params - e.g. explorer or anything with tables or filters. Merging query params would send all of explorer's query params to the new page.


@Input()
public config!: NavItemLinkConfig;

Expand All @@ -68,6 +70,7 @@ export class NavItemComponent {
if (this.config.pageLevelTimeRangeIsEnabled && this.config.timeRangeResolver) {
return {
...navParams,
queryParamsHandling: NavItemComponent.QUERY_PARAM_HANDLING_STRATEGY,
queryParams: this.timeRangeService.toQueryParams(this.config.timeRangeResolver(), true)
};
}
Expand Down