HiveBrain v1.2.0
Get Started
← Back to all entries
patterntypescriptMinor

Modal component in Angular 2

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
modalangularcomponent

Problem

I've recently started to check out Angular 2 and TypeScript that comes with it, and I guess I'm kinda into it. However I'm not too sure about the whole "TypeScript way" of doing things, specifically the typing of variables and such.

I wrote this simple modal component which displays a modal when the ModalApiService has its open function called from anywhere in the application. There can be multiple modals on the same page that's why I had to also include the passing of a modalId to the modal so I could identify them correctly.

  • Can this code be optimized in any way?



  • Do I set the types in a correct way? Are there more things I should set a type for?



  • Does TypeScript have any features that I'm not taking full advantage of here?



Component:

export class ModalComponent {
  public isOpen: boolean = false;

  constructor(private _modalApi: ModalApiService, private _elementRef: ElementRef) {

    let overlay = document.getElementsByClassName('modal-overlay')[0];

    if (overlay) {

      overlay.addEventListener('click', (e: any) => {

        // Close the modal if a click is made on the modal overlay itself, not its children
        if ((e.target.getAttribute('class') || []).indexOf('modal-overlay') !== -1) {
          _modalApi.close(this.modalId);
        }
      });
    }

    _modalApi.isOpen$.subscribe(
      modal => {
        this.isOpen = modal.id === this.modalId ? modal.isOpen : false;
      }
    )
  }
}


Component API service:

@Injectable()

export class ModalApiService {
  private _isOpen = new Subject();

  isOpen$ = this._isOpen.asObservable();

  open(id: string) {
    this._isOpen.next({isOpen: true, id: id});
  }

  close(id: string) {
    this._isOpen.next({isOpen: false, id: id});
  }
}

Solution

Cool stuff, though instead of declaring _isOpen property as a ` in the service class, you should provide a new interface (as you added modalId`).
Something like:

interface IModalElementStatus {
    isOpen: boolean;
    id: string;
}

Code Snippets

interface IModalElementStatus {
    isOpen: boolean;
    id: string;
}

Context

StackExchange Code Review Q#125180, answer score: 3

Revisions (0)

No revisions yet.