Skip to content

feat: support for specifying navigation params to open the page in a new tab where applicable #1503

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 7 commits into from
Mar 30, 2022
Merged

feat: support for specifying navigation params to open the page in a new tab where applicable #1503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b715f87
feat(sheet overlay): support for nav params config
arjunlalb Mar 25, 2022
2f3c37c
fix: test fix
arjunlalb Mar 25, 2022
ed6d691
fix: prettier fix
arjunlalb Mar 25, 2022
9007b76
Merge branch 'main' into open-in-new-tab-support-sheet
arjunlalb Mar 25, 2022
dc2cf23
Merge branch 'main' into open-in-new-tab-support-sheet
arjunlalb Mar 26, 2022
bce6f3d
Merge branch 'main' into open-in-new-tab-support-sheet
arjunlalb Mar 29, 2022
17e43b8
Merge branch 'main' into open-in-new-tab-support-sheet
arjunlalb Mar 30, 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
6 changes: 6 additions & 0 deletions projects/components/src/overlay/sheet/sheet-overlay.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ $box-shadow: -3px 0px 24px rgba(0, 0, 0, 0.12), -1px 0px 8px rgba(0, 0, 0, 0.08)
@include header-4($gray-7);
margin: 0;
}

.action-buttons {
display: flex;
align-items: center;
gap: 24px;
}
}

.content-wrapper {
Expand Down
33 changes: 30 additions & 3 deletions projects/components/src/overlay/sheet/sheet-overlay.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { ChangeDetectionStrategy, Component, Inject, Injector, Optional } from '@angular/core';
import { GLOBAL_HEADER_HEIGHT, LayoutChangeService } from '@hypertrace/common';
import { POPOVER_DATA } from '@hypertrace/components';
import {
ExternalNavigationWindowHandling,
GLOBAL_HEADER_HEIGHT,
LayoutChangeService,
NavigationParamsType
} from '@hypertrace/common';
import { OpenInNewTabComponent, POPOVER_DATA } from '@hypertrace/components';
import { createHostFactory, mockProvider, Spectator } from '@ngneat/spectator/jest';
import { MockComponent } from 'ng-mocks';
import { ButtonComponent } from '../../button/button.component';
Expand All @@ -26,7 +31,7 @@ describe('Sheet Overlay component', () => {

const createHost = createHostFactory({
component: SheetOverlayComponent,
declarations: [MockComponent(ButtonComponent), TestComponent],
declarations: [MockComponent(ButtonComponent), TestComponent, MockComponent(OpenInNewTabComponent)],
shallow: true,
template: `
<ht-sheet-overlay>
Expand Down Expand Up @@ -151,4 +156,26 @@ describe('Sheet Overlay component', () => {

expect(spectator.query(TestComponent)?.data).toBe(42);
});

test('should show open in new tab button if applicable', () => {
spectator = createConfiguredHost({
pageNavParams: {
navType: NavigationParamsType.External,
url: '/test',
windowHandling: ExternalNavigationWindowHandling.NewWindow
}
});

expect(spectator.query(OpenInNewTabComponent)?.paramsOrUrl).toEqual({
navType: NavigationParamsType.External,
url: '/test',
windowHandling: ExternalNavigationWindowHandling.NewWindow
});
});

test('should not show open in new tab button if config is empty', () => {
spectator = createConfiguredHost();

expect(spectator.query(OpenInNewTabComponent)).not.toExist();
});
});
27 changes: 17 additions & 10 deletions projects/components/src/overlay/sheet/sheet-overlay.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ChangeDetectionStrategy, Component, HostListener, Inject, Injector, TemplateRef, Type } from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { GLOBAL_HEADER_HEIGHT, LayoutChangeService } from '@hypertrace/common';
import { ExternalNavigationParams, GLOBAL_HEADER_HEIGHT, LayoutChangeService } from '@hypertrace/common';
import { ButtonStyle } from '../../button/button';
import { IconSize } from '../../icon/icon-size';
import { PopoverFixedPositionLocation, POPOVER_DATA } from '../../popover/popover';
Expand All @@ -18,14 +18,20 @@ import { SheetOverlayConfig, SheetSize } from './sheet';
<ng-container *ngIf="!this.isViewCollapsed">
<div *ngIf="this.showHeader" class="header">
<h3 class="header-title">{{ sheetTitle }}</h3>
<ht-button
class="close-button"
icon="${IconType.CloseCircle}"
display="${ButtonStyle.Outlined}"
htTooltip="Close Sheet"
(click)="this.close()"
>
</ht-button>
<div class="action-buttons">
<ht-open-in-new-tab
*ngIf="this.navigationParams"
[paramsOrUrl]="this.navigationParams"
></ht-open-in-new-tab>
<ht-button
class="close-button"
icon="${IconType.CloseCircle}"
display="${ButtonStyle.Outlined}"
htTooltip="Close Sheet"
(click)="this.close()"
>
</ht-button>
</div>
</div>
<div class="content-wrapper">
<div class="content">
Expand Down Expand Up @@ -61,6 +67,7 @@ export class SheetOverlayComponent {
public readonly closeOnEscape: boolean;
public readonly attachedTriggerTemplate?: TemplateRef<unknown>;
public isViewCollapsed: boolean;
public navigationParams: ExternalNavigationParams | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be InAppNavigationParams. Or you can keep just NavigationParams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anandtiwary open in new tab supports only external nav params. Hence the choice of types.

Alternative would be to use open in new tab conditionally if nav type is internal. But that can be added in a future iteration if we have a case for it. Currently all our hyperlinking cases from sheets are for opening in new tabs / external.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we might be opening an internal link right? So, can we update open-in-new-tab to accept NavigationParam ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anandtiwary Internal nav links are also converted to ExternalNavParams to OpenInNewTab currently.

If we want to extend support for InternalNavParams by default in OpenInNewTab, we can add support for that separately IMO. If you'd like that to be added, I can take that up after this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Sure. We can do it follow up


public constructor(
private readonly popoverRef: PopoverRef,
Expand All @@ -80,7 +87,7 @@ export class SheetOverlayComponent {
this.renderer = sheetConfig.content;
this.popoverRef.height(this.getHeightForPopover(globalHeaderHeight, sheetConfig.position));
this.setWidth();

this.navigationParams = sheetConfig.pageNavParams;
this.rendererInjector = Injector.create({
providers: [
{
Expand Down
3 changes: 2 additions & 1 deletion projects/components/src/overlay/sheet/sheet-overlay.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { ButtonModule } from '../../button/button.module';
import { IconModule } from '../../icon/icon.module';
import { OpenInNewTabModule } from '../../open-in-new-tab/open-in-new-tab.module';
import { TooltipModule } from '../../tooltip/tooltip.module';
import { SheetOverlayComponent } from './sheet-overlay.component';

@NgModule({
imports: [CommonModule, ButtonModule, TooltipModule, IconModule],
imports: [CommonModule, ButtonModule, TooltipModule, IconModule, OpenInNewTabModule],
declarations: [SheetOverlayComponent],
exports: [SheetOverlayComponent]
})
Expand Down
2 changes: 2 additions & 0 deletions projects/components/src/overlay/sheet/sheet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { InjectionToken, TemplateRef } from '@angular/core';
import { ExternalNavigationParams } from '@hypertrace/common';
import { Observable } from 'rxjs';
import { PopoverFixedPositionLocation } from '../../popover/popover';
import { OverlayConfig } from './../overlay';
Expand All @@ -10,6 +11,7 @@ export interface SheetOverlayConfig<TData = unknown> extends OverlayConfig {
closeOnEscapeKey?: boolean;
closeOnNavigation?: boolean;
attachedTriggerTemplate?: TemplateRef<unknown>;
pageNavParams?: ExternalNavigationParams;
}

export const enum SheetSize {
Expand Down