isaacplmann / ngx-contextmenu

An Angular component to show a context menu on an arbitrary component
MIT License
248 stars 91 forks source link

Dynamic context menu open issue #95

Open sherlock1982 opened 6 years ago

sherlock1982 commented 6 years ago

Currently I need to use dynamic context menu like this (approximately):


    public onContextMenu($event: MouseEvent, contact: Contact): void {

        $event.preventDefault();
        $event.stopPropagation();

        this.getContextMenuAction(contact).subscribe( (actions: ContextMenuAction[]) => {
            this.contextMenuActions = actions;
            setTimeout(() => {
                this.contextMenuService.show.next({
                    // Optional - if unspecified, all context menu components will open
                    contextMenu: this.basicMenu,
                    event: $event,
                    item: contact,
                });
            });
        })
    }

onContextMenu by itself is a click event somewhere on a button.

<context-menu #basicMenu>
    <ng-template *ngFor="let action of contextMenuActions" contextMenuItem (execute)="action.execute()">
        <i class="{{action.icon}}"></i><span>{{action.translationTag | translate:action.translationArgs}}</span>
    </ng-template>
</context-menu>

Though if I remove setTimeout two issues appear:

  1. First time context menu is shown without items.
  2. execute is not fired though menu is closed on click

Is there a reason for setTimeout call or am I doing something wrong?

isaacplmann commented 6 years ago

Short answer: Yes, you need the setTimeout.

Long answer: I haven't seen anyone try to do something like this, but here's what I'm guessing is happening.

Without the setTimeout:

  1. this.contextMenuAction is initially undefined (or [] if you set it to that)
  2. <context-menu> sees no contextMenuItems
  3. Setting this.contextMenuActions does not immediately trigger change detection on the <context-menu> so it uses the old values.
  4. The first click shows no items.

With the setTimeout:

  1. and 2. same.
  2. Having setTimeout allows a change detection cycle to run in the <context-menu> component so that it picks up the new <ng-template contextMenuItem>s.
  3. The first click shows the new items.

I have no good explanation for why the execute isn't fired.

sherlock1982 commented 6 years ago

Thank you for the explanation. What I was trying to achieve is this simple scenario:

  1. User clicks on some element
  2. Some async requests might step in to form contextMenuActions.
  3. Context menu opens with generated contextMenuActions.

I think it might make sense to update your example with Dynamic context menu. Cause it actually demonstrates static context menu. It's important that after contextMenuActions are generated new change cycle should take place.

isaacplmann commented 6 years ago

Yes, that's a valid use case and I agree that the <context-menu> should handle it more elegantly, but it currently doesn't. Want to submit a PR to update the docs? I'm always looking for help and you can write clear explanations.

bergermanuel commented 4 years ago

We're having the same issue with using dynamic context menus. When looking at the code we identified the issue being the ContentChildren Input inside the ContextMenuDirective. As you mentioned it's the changeDetection that didn't update the values between setting the items and opening the context menu.

We also tried to call the changedetection manually between the two lines of code but didn't work. Also calling setTimeout is not working properly. We have to use a specific time of milisecods to get it working.

      this.contextMenuActions = actions;
      this.triggerChange();

      if (!actions || !actions.length || actions.every(a => a.hidden)) {
        return;
      }

      setTimeout(() => {
        this.contextMenuService.show.next({
          event: event.event,
          contextMenu: this.contextMenuComponent,
          item: null
        });
        this.triggerChange();
      }, 450);

In my opinion it's a very common use case to have one contextmenu and setting the entries dynamically with an ngFor. We're looking for a clean way to solve this, do you have any suggestions for us?