Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
8bf3da5
fix: donut chart fixes
itssharmasandeep Jul 13, 2021
5599c6a
Revert "fix: donut chart fixes"
itssharmasandeep Jul 13, 2021
88d3bd4
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Jul 15, 2021
cd18474
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Jul 19, 2021
2aa1a9b
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Jul 20, 2021
111e41d
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Jul 26, 2021
9b40fc4
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Jul 28, 2021
00b1e07
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Jul 29, 2021
f81b19d
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Jul 30, 2021
423dc6d
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Aug 9, 2021
9466514
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Aug 11, 2021
b2b04ba
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Aug 18, 2021
1e2e570
Merge remote-tracking branch 'origin/main'
itssharmasandeep Aug 19, 2021
403a9d4
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Aug 20, 2021
a74bd5d
Merge remote-tracking branch 'origin/main'
itssharmasandeep Aug 23, 2021
5d47c7f
Merge branch 'main' of github.com:hypertrace/hypertrace-ui
itssharmasandeep Aug 25, 2021
60746c3
feat: custom dimensions for modal
itssharmasandeep Aug 25, 2021
94374ed
fix: test case
itssharmasandeep Aug 25, 2021
29c7ed5
fix: addressed review comments
itssharmasandeep Aug 25, 2021
d5fc366
fix: lint error
itssharmasandeep Aug 25, 2021
cf7774e
fix: add test
itssharmasandeep Aug 25, 2021
d6d65ad
fix: build error
itssharmasandeep Aug 25, 2021
b14e30a
fix: remove unwanted code
itssharmasandeep Aug 25, 2021
f2084a1
fix: add test
itssharmasandeep Aug 25, 2021
ed028d6
fix: lint errors
itssharmasandeep Aug 25, 2021
c5a2226
fix: code refactor
itssharmasandeep Aug 26, 2021
5c7bfdd
fix: test cases and code refactor
itssharmasandeep Aug 26, 2021
35d133a
Merge branch 'main' into custom-modal-dimension
itssharmasandeep Aug 30, 2021
a3826cb
Merge remote-tracking branch 'origin/main' into custom-modal-dimension
itssharmasandeep Sep 1, 2021
b18ede0
fix: addressing review comments
itssharmasandeep Sep 1, 2021
f3dc565
Merge branch 'main' into custom-modal-dimension
itssharmasandeep Sep 1, 2021
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
30 changes: 0 additions & 30 deletions projects/components/src/modal/modal-container.component.scss
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,6 @@
display: flex;
flex-direction: column;

&.modal-size-small {
height: 365px;
width: 420px;
}

&.modal-size-medium {
height: 530px;
width: 456px;
}

&.modal-size-large-short {
height: 540px;
width: 640px;
}

&.modal-size-large {
height: 720px;
width: 640px;
}

&.modal-size-large-tall {
height: 800px;
width: 640px;
}

&.modal-size-medium-wide {
height: 600px;
width: 840px;
}

.header {
display: flex;
flex-direction: column;
Expand Down
73 changes: 71 additions & 2 deletions projects/components/src/modal/modal-container.component.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ class TestComponent {
describe('Modal Container component', () => {
let spectator: Spectator<ModalContainerComponent>;

const checkSyles = (width: string, height: string): void => {
const modalContainer = spectator.query('.modal-container') as HTMLElement;
expect(modalContainer.style.height).toBe(height);
expect(modalContainer.style.width).toBe(width);
};

const createHost = createHostFactory({
component: ModalContainerComponent,
shallow: true,
Expand Down Expand Up @@ -77,14 +83,77 @@ describe('Modal Container component', () => {
expect(spectator.query('.header')).toHaveText('Create User');
});

test('uses the requested size', () => {
test('uses the requested small size', () => {
spectator = createConfiguredHost({
showControls: true,
title: 'Create User',
content: TestComponent,
size: ModalSize.Small
});
expect(spectator.query('.modal-container')).toHaveClass('modal-size-small');
checkSyles('420px', '365px');
});

test('uses the requested medium size', () => {
spectator = createConfiguredHost({
showControls: true,
title: 'Create User',
content: TestComponent,
size: ModalSize.Medium
});
checkSyles('456px', '530px');
});

test('uses the requested large-short size', () => {
spectator = createConfiguredHost({
showControls: true,
title: 'Create User',
content: TestComponent,
size: ModalSize.LargeShort
});
checkSyles('640px', '540px');
});

test('uses the requested large size', () => {
spectator = createConfiguredHost({
showControls: true,
title: 'Create User',
content: TestComponent,
size: ModalSize.Large
});
checkSyles('640px', '720px');
});

test('uses the requested large-tall size', () => {
spectator = createConfiguredHost({
showControls: true,
title: 'Create User',
content: TestComponent,
size: ModalSize.LargeTall
});
checkSyles('640px', '800px');
});

test('uses the requested medium-wide size', () => {
spectator = createConfiguredHost({
showControls: true,
title: 'Create User',
content: TestComponent,
size: ModalSize.MediumWide
});
checkSyles('840px', '600px');
});

test('custom size', () => {
spectator = createConfiguredHost({
showControls: true,
title: 'Create User',
content: TestComponent,
size: {
width: 100,
height: 100
}
});
checkSyles('100px', '100px');
});

test('closes on close button click', () => {
Expand Down
28 changes: 24 additions & 4 deletions projects/components/src/modal/modal-container.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ import { LayoutChangeService } from '@hypertrace/common';
import { ButtonSize, ButtonStyle } from '../button/button';
import { POPOVER_DATA } from '../popover/popover';
import { PopoverRef } from '../popover/popover-ref';
import { ModalConfig, ModalSize, MODAL_DATA } from './modal';
import { getModalDimensions, ModalConfig, ModalDimension, MODAL_DATA } from './modal';

@Component({
selector: 'ht-modal-container',
styleUrls: ['./modal-container.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<div *ngIf="this.visible" class="modal-container" [ngClass]="'modal-size-' + this.size">
<div *ngIf="this.visible" class="modal-container" [style.height]="this.size.height" [style.width]="this.size.width">
<div class="header">
<ht-button
*ngIf="this.showControls"
Expand Down Expand Up @@ -41,7 +41,7 @@ import { ModalConfig, ModalSize, MODAL_DATA } from './modal';
export class ModalContainerComponent {
public readonly modalTitle: string;
public readonly showControls: boolean;
public readonly size: ModalSize;
public readonly size: ModalDimension;
public readonly isComponentModal: boolean;
public readonly renderer: TemplateRef<unknown> | Type<unknown>;
public readonly rendererInjector: Injector;
Expand All @@ -58,7 +58,9 @@ export class ModalContainerComponent {
const config = constructionData.config;
this.showControls = config.showControls ?? false;
this.modalTitle = config.title ?? '';
this.size = config.size;
this.size = this.isModalDimension(config.size)
? this.getDimensionsWithUnits(config.size)
: this.getDimensionsWithUnits(getModalDimensions(config.size));
this.isComponentModal = !(config.content instanceof TemplateRef);
this.closeOnEscape = config.closeOnEscapeKey ?? true;
this.renderer = config.content;
Expand All @@ -85,6 +87,24 @@ export class ModalContainerComponent {
this.visible = false;
this.popoverRef.close();
}

private isModalDimension(size: unknown): size is ModalDimension {
Comment thread
itssharmasandeep marked this conversation as resolved.
// tslint:disable-next-line:strict-type-predicates
return typeof size === 'object' && size !== null && 'width' in (size as ModalDimension);
}

private getDimensionsWithUnits(dims: ModalDimension): ModalDimension {
const dimsWithUnits: ModalDimension = { ...dims };
if (typeof dims.height === 'number') {
dimsWithUnits.height = `${dims.height}px`;
}

if (typeof dims.width === 'number') {
dimsWithUnits.width = `${dims.width}px`;
}

return dimsWithUnits;
}
}

export interface ModalConstructionData {
Expand Down
29 changes: 28 additions & 1 deletion projects/components/src/modal/modal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { InjectionToken, TemplateRef, Type } from '@angular/core';
import { Observable } from 'rxjs';

export interface ModalConfig<TData = unknown> {
size: ModalSize;
content: TemplateRef<unknown> | Type<unknown>;
size: ModalSize | ModalDimension;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm... this kind of makes the design weird. The same property is trying to do two things.

What if, we introduce a new enum in ModalSize called CustomSize and then add another optional property for dimension inputs? Then we can keep the code cleaner.

Alternatively, the config itself can change for custom size.

@aaron-steinfeld thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I disagree. It's serving the same role in either case - a way to specify the size. Every other option is the same, so it doesn't make sense to have two versions of the config or an optional property (since the optional property is meaningless depending on the enum value). This is the beauty of unions - it's a strictly easier API for the caller to work with. Any messiness is internal to the implementation, and disambiguating between types should really just be a one liner.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fine. I don't have strong feelings. I find it weird when we put type based logic in the code. IMO, keeping separate properties makes it cleaner from API perspective. But it is a preference

showControls?: boolean;
title?: string;
data?: TData;
Expand All @@ -19,6 +19,33 @@ export const enum ModalSize {
MediumWide = 'medium-wide'
}

export interface ModalDimension {
// Number => without unit (considered px) and String => with units (expression included)
width: number | string;
Copy link
Copy Markdown
Contributor

@anandtiwary anandtiwary Aug 30, 2021

Choose a reason for hiding this comment

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

What if I pass a random string?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I mean, it won't work at that time. And I think this should be the case. Like suppose if we do this in the css (putting wrong value for width), then it doesn't work. So It is developer's responsibility to put correct value and I think developer can handle this.

Copy link
Copy Markdown
Contributor

@aaron-steinfeld aaron-steinfeld Aug 31, 2021

Choose a reason for hiding this comment

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

If you want to get really fancy, TS supports this kind of thing now. I'd also be fine just taking in any string, we don't validate to nearly this level elsewhere.

type Unit = "px" | "em" | "fr" | "%"; // probably some others too

type SizeWithUnit = `${number}${Unit}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in action:
image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we could do this, but my intention of using string, to use these kind of expression as well, not only simple units - calc(100vh - 100px)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

type SizeExpression = SizeWithUnit | `calc(${SizeWithUnit} - ${SizeWithUnit})`

Typescript type system is turing complete 😉

But seriously, yes I agree, overkill.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah!!!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I will let you guys decide :D

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

According to me, It is good enough and as @aaron-steinfeld said doing this, is an overkill.
I think we can merge this.

height: number | string;
}

export const getModalDimensions = (modalSize: ModalSize): ModalDimension => {
switch (modalSize) {
case ModalSize.Small:
return getModalDimensionObject(420, 365);
case ModalSize.Medium:
return getModalDimensionObject(456, 530);
case ModalSize.LargeShort:
return getModalDimensionObject(640, 540);
case ModalSize.Large:
return getModalDimensionObject(640, 720);
case ModalSize.LargeTall:
return getModalDimensionObject(640, 800);
case ModalSize.MediumWide:
return getModalDimensionObject(840, 600);
default:
return getModalDimensionObject(420, 365);
}
};

const getModalDimensionObject = (width: number, height: number): ModalDimension => ({ width: width, height: height });

export const MODAL_DATA = new InjectionToken<unknown>('MODAL_DATA');

export abstract class ModalRef<TResult> {
Expand Down