Skip to content
This repository has been archived by the owner on Apr 5, 2023. It is now read-only.

Modal would be more useful with a dismiss method that accept a reason of type any #59

Closed
ghiscoding opened this issue Sep 8, 2018 · 17 comments

Comments

@ghiscoding
Copy link

ghiscoding commented Sep 8, 2018

Started using your great library and it's working great, however it would be even better if the Modal Component could have a dismiss(reason: any) (or close) method. As for example, I am working on a modal window that has a form and I need to be able to use the form data into the parent. For a better example, let say we have a user which create an item and once the item is saved, we close the modal window and we use the object returned by the server to refresh the grid (without refreshing the entire grid, we just add that 1 object to the grid via typescript).

I'm not totally sure on how to implement in your component but if we look at ng-bootstrap, they have the dismiss method. In their example, click on "Launch demo modal" and then click on the "Save" button and you will see the text "Closed with: Save click" appearing on the parent window via the dismiss(reason: any) method.

Currently your Modal Component can only be closed via the View, I don't see how to close the modal window in ViewModel (.ts) code.

For now, I guess I could do the same via an Event Aggregator, something like this.ea.publish('onItemCreated', this.item);, but that would be great if the Modal Component support the dismiss(reason: any) method internally.

@ghiscoding
Copy link
Author

After thinking about it, this could be done via a Custom Event that can be dispatched. For the dismiss "reason", we might need an extra bindable property that can be named data (or dismissReason).

So the Modal Custom Component could add the dismiss Custom Event that would be dispatching the reason.

@customElement('abt-modal')
export class BootstrapModal {
  // ...

  // a new property to hold the modal data or the dismiss reason
  @bindable({ defaultBindingMode: bindingMode.twoWay }) data: any;

  // dismiss method, can be called by visibleChanged false
  private dismiss() {
    this.modal.dispatchEvent(new CustomEvent(`dismiss`, {
        bubbles: true,
        detail: this.data // or dismissReason
    }));
  }

  // when modal is hiding, we can call the dismiss function
  private visibleChanged(newValue: string | boolean) {
    // ... 

    $(this.modal).modal('hide');
    this.dismiss();
  }

Then in the code, we could use it this way in the View

<abt-modal open-by="showTransactionEdit" dismiss.delegate"addItemToGrid($event.detail)">

And in the ViewModel (.ts)

export class MyClass {
  addItemToGrid(newItem) {
    // newItem is the data coming from the modal form (or in other word the dismiss reason)
    console.log('New Item Object: ', newItem);
  }
}

If it's too confusing, I can maybe make a PR

@shahabganji
Copy link
Contributor

shahabganji commented Sep 9, 2018

@ghiscoding
Today I am so busy, I'll take a look at it, but off the top of my head we had bs-hide and bs-hidden methods, can't they be of any help? you could find them in the doc at the very bottom of the page.

<abt-button id="eventsModal">
  Launch demo modal
</abt-button>

<!-- Modal -->
<abt-modal open-by="eventsModal" bs-show.call="showEvent()" bs-hide.call="hideEvent()">
  <abt-modal-header>
    <abt-modal-title class="modal-title-aurelia">
      <h5>Aurelia Toolbelt Dialog</h5>
    </abt-modal-title>
  </abt-modal-header>
  <abt-modal-body>
    <div class="container">
      Check Browser
      <b>console</b>
    </div>
  </abt-modal-body>
</abt-modal>
export class BootstrapListGroup {
  private showEvent() {
    console.log('Modal show');
  }

  private hideEvent() {
    console.log('Modal hide');
  }
}

@shahabganji
Copy link
Contributor

shahabganji commented Sep 9, 2018

@ghiscoding

If the above-mentioned approach does not suffice, I would be more than happy to accept your PR.

@ghiscoding
Copy link
Author

ghiscoding commented Sep 9, 2018

@shahabganji
Thanks for the reply, and don't worry I was not asking to have this right away. I was only suggesting for future improvement :)

As far as I know, bs-hide have different purpose and we cannot pass extra arguments to these methods. I would like to use Custom Event, because we can pass arguments to it and that is basically what I am asking for. I want to dismiss (close) the modal and pass form data to the parent window.

For example, let say that the parent window is a User List which has a modal component to create a new User, once the User is created and saved, I would like to pass this object (form data) back to the User List (parent window) and update the User List with this new user (without refreshing the entire User List).

Most of the code is shown in my previous post. I can look at creating a PR in the coming days. Thanks

@shahabganji
Copy link
Contributor

shahabganji commented Sep 9, 2018

@ghiscoding

I made my hands dirty on dismiss event, it works properly, however, I need to check some stuff before publishing:

  1. What if the modal lets you return arguments from its bs-hidden event, it's against bootstrap's hidden.bs.modal event though but will not hurt anyone. ( Personally, I prefer NOT to, I think it's better to have the same signature as far as possible )

  2. In your example don't you have a new_user object in the parent element? AFAIK bootstrap's modal is in the same page as its parent:

  • The view-model and the model would be as following:
interface IUser {
  firstName: String;
  lastName: String;
}

export class BootstrapModalDemo {
  private showModal = false;

  private user: IUser = { firstName: '', lastName: '' };
  private users: Array<IUser> = [
    { firstName: 'Saeed', lastName: 'Ganji' },
    { firstName: 'Hamed', lastName: 'Fathi' }
  ];

  private dismiss(data: IUser) {
    this.user = data;

    let u = {};
    Object.assign(u, data);
    this.users.push(<IUser>u);
  }
}
 <ul>
    <li repeat.for="usr of users">
      ${usr.firstName} - ${usr.lastName}
    </li>
  </ul>

  <abt-button id="btnPersonModal">
    Show Person Modal
  </abt-button>

  <abt-modal dismiss.delegate="dismiss($event.detail)" open-by="btnPersonModal" data.bind="user">
    <abt-modal-header>
      A modal with dismiss event handled
    </abt-modal-header>
    <abt-modal-body>
      <div>
        <label>
          First Name:
        </label>
        <input value.bind="user.firstName" />
        <label>
          Last Name:
        </label>
        <input value.bind="user.lastName" />
      </div>
    </abt-modal-body>
  </abt-modal>

Now, may I ask you to describe more why there is such a requirement? Because I guess that it is possible to easily change

 Object.assign(u, data);

to

Object.assign(u, this.user);
  1. How should I assign values to the new bindable property data ? and should I use with binding inside the modal?

@ghiscoding
Copy link
Author

ghiscoding commented Sep 10, 2018

@shahabganji
Wow you got ahead of me, I had some thought and also testing some piece of it.

  1. Yes I agree, it's better to keep Bootstrap method with same signature to avoid confusion. Which is why I proposed the dismiss method (which is used in Angular on ng-bootstrap)
  2. Actually no I always prefer to separate my code as much as possible (separation of concerns), so I create the Modal through a compose in the parent window, that goes along:
<compose view-model="./create"></compose>

So I don't have direct access to the new_user which is why I was using at first Event Aggregator, however today I found a few ways of passing the data back to the parent window (user list). From the example below (number 3 to 5), I found how to do this code from this article.

I'm not sure this is ideal but I manage to do it this way:

From a User List (parent component), I use compose to create the modal window (child component)
User List (HTML)

<template>
  <compose view-model="./createUser"></compose>

  <div class="container">
      <div class="alert alert-info" show.bind="newTransaction">
        <strong>New User:</strong> ${newUser | stringify}
      </div>
    </div>

  <h4>User List</h4>
  1. One way is to use DI to inject the UserList (parent component) in the CreateUser (child component), and to my surprise that works, wow amazing.
Create User (ts) / Modal Component
import { autoinject, bindable } from "aurelia-framework";
import { UserDataService } from "./userDataService";
import { UserList } from './userList';

@autoinject()
export class CreateUser {
  constructor(private userList: UserList, private userDataService: UserDataService) { }

  save() {
    // hide the modal & pass new created user to the user list parent component
    this.showCreateModal = false; 
    this.list.newTrsn = this.transaction;
  }
}
  1. A 2nd way is to use bind to get access to the Parent from the Child
Create User (ts) / Modal Component

We can use DI to inject the UserList (parent component)

@autoinject()
export class CreateUser {
  bind(bindingContext, overrideContext) {
    this.parent = overrideContext.parentOverrideContext.bindingContext;
  }

  save() {
    // hide the modal & pass new created user to the user list parent component
    this.showCreateModal = false; 
    this.parent.newTrsn = this.transaction;
  }
}
  1. and yet a 3rd way is to use the compose with model.bind to have a reference of the parent instance.
User List (parent html)
<template>
  <compose view-model="./createUser" model.bind="thisList"></compose>
</template>
User List (parent ts)
export class TransactionList {
  thisList = this;

then use activate method to get parent reference

Create User (ts) / Modal Component

We can use DI to inject the UserList (parent component)

@autoinject()
export class CreateUser {
  activate(parentelement) {
    this.parent = parentelement;
  }

  save() {
    // hide the modal & pass new created user to the user list parent component
    this.showCreateModal = false; 
    this.parent.newTrsn = this.transaction;
  }
}

Final Word

Considering, I found today how to access the Parent Component (user list) as number 3-5 shows. I'm wondering if dismiss will be of great use anymore.

I did not make any big projects yet with Aurelia, so I'm wondering what is the best approach to use in this case. I think number 3 might not be good to use, but 4 and 5 look interesting.

What are your thoughts and do you see more reasons to use the dismiss method or use something that I have shown in this post? What looks best for you?

Thanks for all :)

oh by the way, what are you using to show Aurelia HTML code in GitHub? I use the 3 backticks ``` html but a lot of the code is showing in red. What do you use to make show correctly here?

@shahabganji
Copy link
Contributor

shahabganji commented Sep 11, 2018

@ghiscoding

This week, unfortunately, I am so busy. I will deep dive into what you just said, however, I had an idea of integrating abt-modal with aurelia-dialog-service from the early days of aurelia-toolbelt, I didn't get the chance though. I think if I could implement that, will it bring us both separation of concern and easier means of communication between parents and child components without the complexities of compose, might need some help though.

what do you think? any suggestions?

@ghiscoding
Copy link
Author

@shahabganji
There's no need to excuse yourself, you already passed a lot of time supporting this great library.

I actually never tried the Aurelia-Dialog, I was not sure if it was a simple dialog or a modal component. I also have to admit that so far, all the Aurelia project is for personal use since my work decided to go with Angular (which I disagreed and tried to push for Aurelia). So I'm a bit limited on the Aurelia knowledge and usage but I'm trying hard :)

@shahabganji
Copy link
Contributor

@ghiscoding

This is a working sample of bootstrap dialogs in conjunction with aurelia-dialog service.

I am trying to come up with a solution.

@shahabganji shahabganji changed the title Modal would be more useful with a dismiss method that accept a reason of type any WIP: Modal would be more useful with a dismiss method that accept a reason of type any Sep 12, 2018
shahabganji added a commit that referenced this issue Sep 14, 2018
@shahabganji
Copy link
Contributor

@ghiscoding

I have integrated abt-modal with aurelia-dialog plugin.

In docs scroll down to find a working sample, like your above-mentioned sample I guess, and a prompt dialog also.

Feel free to reopen this thread, if you hit any bug. version 1.1.0 released on npm too.

@ghiscoding
Copy link
Author

Wow very nice, it looks like exactly what I need, thank you so much. 😃

@shahabganji
Copy link
Contributor

@ghiscoding can you provide me some feedbacks, sounds other developers have some problems with the new release, would you mind check it with your own project ?

@ghiscoding
Copy link
Author

ghiscoding commented Sep 14, 2018

@shahabganji
Trying it over lunch time, I updated to your latest version 1.1.1 and I don't see any errors in the console or from any components. I did not try your new modal with aurelia-dialog yet (I will try that tomorrow).

What kind of problems does the other users sees? Never mind, I just saw the other issue opened and I left a message.

@shahabganji shahabganji changed the title WIP: Modal would be more useful with a dismiss method that accept a reason of type any Modal would be more useful with a dismiss method that accept a reason of type any Sep 17, 2018
@ghiscoding
Copy link
Author

ghiscoding commented Oct 9, 2018

@shahabganji
I finally got the aurelia-dialog working with WebPack 4, I literally passed few days on this and of course it was 1 simple line of code to be the problem.

So I got the Prompt aurelia-dialog sample working and it's all good, except when I try to add your Modal demo mixed with Aurelia-Dialog, that doesn't seem to work properly. It seems that something happens (a hidden modal). Would you mind taking a look at the fork of your starter kit, I made a fork and simply try adding the modal sample into it (it also has the Prompt like I said earlier). The fork is available at this link. Is there supposed to be some <ux-dialog></ux-dialog> code somewhere? I looked at your code implementation in your commit and didn't see that anywhere, is that normal?

See below for a demo, when I click "Add Developper", it looks like it's a hidden (or white) modal dialog, I cannot click on the button in the back and I have to click "ESC" on my keyboard to get the rest of the window working again.

2018-10-08_23-46-07

@shahabganji
Copy link
Contributor

shahabganji commented Oct 9, 2018

@ghiscoding

I'll check it today

Is there supposed to be some code somewhere?

No, it does not, I used a custom renderer


Edit 1: Just a quick look I guess you must comment these lines, however I let you know exactly what's the problem

@shahabganji
Copy link
Contributor

shahabganji commented Oct 9, 2018

@ghiscoding

The root cause of the issue is exactly what I thought ☝️ , you don't need to configure aurelia-dialog plugin, we are using its DialogController and DialogService to keep bootstrap modal work the same as aurelia-dialog, we created a DialogRenderer, and that knows how to adapt abt-modal and aurelia-dialog services such as DialogController and DialogService.

BTW, after commenting the codes in your main.ts replcae the code in your prompt.html with the following:

<template>
  <abt-modal>
    <abt-modal-header>${question}</abt-modal-header>

    <abt-modal-body>
      <form submit.trigger="controller.ok(answer)">
        <input type="text" value.bind="answer" style="width: 100%; line-height: 16px; font-size: 16px" attach-focus>
      </form>
    </abt-modal-body>

    <abt-modal-footer>
      <button click.trigger="controller.cancel()">Cancel</button>
      <button click.trigger="controller.ok(answer)">Ok</button>
    </abt-modal-footer>
  </abt-modal>
</template>

Maybe, we need to mention that developers need NOT to configure aurelia-dialog.

screen shot 2018-10-09 at 10 18 09

@ghiscoding
Copy link
Author

ghiscoding commented Oct 9, 2018

@shahabganji
Thanks a lot for looking into this. Yes you should have a mention somewhere in your doc to not include any aurelia-dialog config. It might also be good to keep this modal component inside your starter kit, I think it's nice to see actual code on the problematic plugins. Thanks for all :)

EDIT
I tried over lunch, I didn't know that I had to completely remove the aurelia-dialog from the main. I thought you meant only it's config. So that definitely should be part of your doc, as I was totally unaware of this. Does that also mean that we can't use aurelia-dialog separately? I don't really care, but just curious to know.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants