Skip to content
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.

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

Added "recycle" option to open #279

Closed
wants to merge 2 commits into from
Closed

Conversation

dboydor
Copy link

@dboydor dboydor commented Jul 22, 2015

To support using ngDialog in a wizard context, I wanted to prevent the unsightly flash of successive dialogs being closed, then opened again.

The solution I came up with, reuses the dialog in-place for a given dialog id. You don't call "close" on the dialog, you just pass a "recycle: [dialogid]" parameter to the "open" call and it will reuse the existing dialog in-place with the new template and controller.

This may not be right for inclusion into ngDialog, but it might be helpful for someone else who wants to support this use case.

@davidvuong
Copy link
Contributor

Hey @dboydor,

I needed the same wizard-like functionality with ngDialog a while ago too. My solution at the time was to maintain states within the template and have ng-show and ng-click to switch between them. So for example:

<div ng-show="!state || state === 'step-1'">
  <button ng-click="state = 'step-2'">next</button>
<div>

<div ng-show="state === 'step-2'">
  <button ng-click="state = 'step-1'">back</button>
</div>

The dialog is created once and you stop having to worry about flashing because you're not closing the dialog here either. You're just toggling on and off steps in the wizard.

I like your solution 👍 but I think it might be a bit tricky to keep bugs at bay when we're re-using existing dialogs. For example I think I spot a bug (I might be wrong here), but it looks like dialogsCount is always incremented even though you might be re-using the same dialog.

@egor-smirnov What do you think?

@dboydor
Copy link
Author

dboydor commented Jul 28, 2015

@egor-smirnov,

I considered doing it the way you suggest (we're using something like it elsewhere) but in this case, we had a hacked route-based dialog solution with a deep branching tree. It would have been painful to merge all that together.

With this solution, I was able to keep the templates and controllers pretty much as-is.

Thanks for noticing the bug! I'll check that out. I'm sure there are others because I'm not using all the features (like resolve) that ngDialog supports and I pretty much ignore those in the reuse case.

@egor-smirnov
Copy link
Member

@dboydor I guess you meant @davidvuong, not me, am I correct? Because I wasn't involved in this discussion by this moment, and haven't reviewed it.

@dboydor
Copy link
Author

dboydor commented Jul 28, 2015

Oh yes! I saw "@egor-smirnov What do you think?" and I thought that was a signature line with him asking me for my comment.

@davidvuong instead

@davidvuong
Copy link
Contributor

Hey @dboydor,

Sorry I haven't been responsive lately. I've been a bit sick (Aussie weather is sporadic ATM) and had been busy with other projects. If this is a use case you need and can't be easily solved by the method I suggested above then I'd be happy to merge your changes in! We just have to hashed out some of the obvious bugs.

@egor-smirnov
Copy link
Member

@davidvuong thank you for your involvement and get well!

You could just work on this PR without asking my opinion. Feel free to merge / discuss on your own.

@davidvuong
Copy link
Contributor

@egor-smirnov will do and thanks!

@davidvuong
Copy link
Contributor

I'm going to close this PR as there hasn't been any activity in over 2 weeks. There are still bugs in the PR and I don't currently have the time to extend it. I'll gladly re-open this later if anyone feels that this feature needs to be incorporated into the library.

@davidvuong davidvuong closed this Aug 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants