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

Modal: consider supporting stacked modals #643

Closed
pkozlowski-opensource opened this issue Aug 31, 2016 · 42 comments
Closed

Modal: consider supporting stacked modals #643

pkozlowski-opensource opened this issue Aug 31, 2016 · 42 comments

Comments

@pkozlowski-opensource
Copy link
Member

That is - modals that can be opened from modals. The current thinking is:

  • we are very reluctant to support those as it is very, very bad UX
  • "real life" sometimes "requires" those anyway....

Will try to resist us long as possible before implementing stacked modals, but let's have this issue to track interest / real-life usage scenarios.

@natcohen
Copy link

I think it should be supported (for your record)

@Quinn-L
Copy link

Quinn-L commented Aug 31, 2016

I use modal for yes/no confirmation dialogs.
However, if stacked modals aren't supported this means you can't have 'normal' modals with data entry which then uses modal confirmation dialogs. I guess one could argue data entry in modals are a bad UX?

@pkozlowski-opensource
Copy link
Member Author

Yeh, I'm kind of resigning to the reality of many projects and bracing for supporting this... But won't get to it before we land datepicker.

@pkozlowski-opensource
Copy link
Member Author

Voice in the discussion: twbs/bootstrap#20607 (comment)

@asadsahi
Copy link

asadsahi commented Sep 8, 2016

I also agree with @pkozlowski-opensource stacked modal aren't good from UX perspective. Also, we had huge number of problems in one of large SPA build in Angular 1. Where there were confirmation dialogs, terms dialogs on top of existing dialogs, which caused more complexity, especially when in a web app reload happens, it is difficult to maintain state for those. We totally scrappeed the idea of having functional templates in modal. only for confirmation dialogs etc.

@tytskyi
Copy link

tytskyi commented Dec 29, 2016

Is there any plans to support backdrop position in between stacked modals?
It turns that for each inner modal its backdrop doesn't cover previous modal.

There is some quick reproduction: http://plnkr.co/edit/FG03lIbHjzM9wAZMoLWL?p=preview

@achronicity
Copy link

@pkozlowski-opensource If this is considered by the team to be "very, very bad UX," what is the suggested alternative when several closely associated components need to operate on the same data? Especially with data that's already loaded? @ViewChild and direct subcomponent access? Child routes (and the terrible API of having to have a singleton service to pass references to in-memory data)?

@ASHFAQPATWARI
Copy link

@pkozlowski-opensource As you already said, sometimes we need stacked modals anyway. What is the alternative approach suggested by you till the library support stacked modals ?

@DocteurEz
Copy link

@pkozlowski-opensource is there any progress on this matter?

@ravtakhar
Copy link

When opening a modal(2) within a modal(1) it doesn't apply a backdrop to modal 1

@dguisinger
Copy link

Not sure where you are at on this issue, but I agree multiple modals are needed. In my instance I'm working on an address component what when you click edit opens a modal (because the edit controls take up much more place than a read-only address displayed in the DOM), I don't want to limit the component to only working outside of modals. In fact, I find it frustrating looking over the documentation that you are building an alternative implementation of Bootstrap.js, but treat modals so entirely different. I don't understand why instead of using divs and modal classes, why we need to use templates and host it in a special tag somewhere in the modal. From what I can tell, none of your other controls are imposing a different design than the standard Bootstrap.

I can understand you wanting to do it a particular way for ease of use for what you consider the common design pattern, but I can't see the justification for eliminating my ability to stick my modals anywhere in my HTML DOM that I chose and open them anyway I please.

@pkozlowski-opensource pkozlowski-opensource removed this from the beta.0 (Feature-complete) milestone Mar 27, 2017
@natcohen
Copy link

natcohen commented May 3, 2017

There is also a problem when the first modal has a scroll bar... when you open a stacked modal over the first one and close it, it becomes impossible to scroll the first one.

Reproduce here: http://plnkr.co/edit/ZGYAbyJXwn8IWqz4sme8?p=preview

  • Open by clicking Launch demo
  • Scroll down and click on the stacked button
  • Close the small modal
  • Try to scroll again, it won't work...

@Vx2gas
Copy link

Vx2gas commented May 26, 2017

Any updates on this? I would like to have the first modal included in the backdrop when the second modal is opened.

Quick fix is to add the following style: Assuming the backdrops/modals are added correctly in the DOM
.modal-backdrop { z-index:1050; }

@dmitrysteblyuk
Copy link

@Vx2gas, z-index helps to some extent (the second modal backdrop is shown now over the first modal), but it doesn't fix the issue with scrolling:
http://plnkr.co/edit/3cISZTeEoqfb76KaC12z?p=preview

@Vx2gas
Copy link

Vx2gas commented Jun 5, 2017

@dmitrysteblyuk, your right. Anyone got a fix for this?

@dmitrysteblyuk
Copy link

dmitrysteblyuk commented Jun 5, 2017

@Vx2gas the issue was that NgbModalRef removes modal-open class from body on destroy (despite that the first modal is still open). I added for myself this quick fix

modalRef.result.catch(() => {})
    .then(() => {
        if (document.querySelector('body > .modal')) {
            document.body.classList.add('modal-open');
        }
    });

@dmitrysteblyuk
Copy link

It seems, adding z-index for modal backdrop and modal-open class for body fixes all the mentioned above issues. I tested only in browser platform though.

@natcohen
Copy link

Anyone for a PR?

@ansorensen
Copy link

An alternative to stacking modals would be the ability to switch between modals.

I attempted to do this manually: have a child component emit an event when it has a modal open. The parent (with it's own modal) could close the parent modal when the child is open, and reopen the parent modal when the child closes. However, calling modalRef.close on the parent appears to detach all modal listeners. The child's modalRef.result.then never fires, so I can't emit an event when the child closes.

Looks like the windowCmptRef gets nullified on close (modal-ref.ts line 98), and windowCmptRef is what is responsible for subscribing to close events (modal-ref.ts line 54). Is it possible that all modals are sharing the same windowCmptRef? It gets assigned at (modal-stack.ts line 52).

Plunkr of the issue here.

@lyonzy
Copy link

lyonzy commented Oct 10, 2017

Just to add my 2c, when you say:

it is very, very bad UX

Twitter themselves use stacked modals - try going to a tweet and clicking on the picture.

@SoftCircuits
Copy link

It's not a great UI/UX, but it's not that terrible. It's done all the times with desktop applications. I recognize that it might be problematic, and I don't know what's involved with the scrolling issue, but it would be nice if this were straight forward to do for situations that call for it.

@pbrain19
Copy link

well...

@earle36
Copy link

earle36 commented Dec 1, 2017

The team I'm on has support use cases for stacked modals regardless of whether its good UI/UX. We've been using ng-bootstrap and love it so far - this is the only issue holding us up right now. I really hope support gets added for this - would be a shame to have to use a different library just because of this one issue.

@Arman92
Copy link

Arman92 commented Dec 18, 2017

I need stacked modals too, any progress regarding this feature?

@ravtakhar
Copy link

@Arman92 what I'm doing as a work around is hiding the parent modal upon open, and then using @dmitrysteblyuk's trick to bring the backdrop back.

@shyamal890
Copy link

@pkozlowski-opensource Can you please update us on the status of this issue and what's stopping the team from implementing this feature?

@shyamal890
Copy link

shyamal890 commented Apr 21, 2018

@pkozlowski-opensource In the latest issue you have stated that until stacked modals are not supported in bootstrap 4 this library won't be supporting it. Can you please let us know what then is the best UI flow for the following workflow?

userslist modal -> user clicks on delete button -> modal confirming deletion

I frankly believe this is the most natural flow and ng-bootstrap being focused on SPA such scenarios would be very frequent.

Moreover, angular-material also provides support for stacked modals. Just because bootstrap 4 doesn't provide this functionality shouldn't be the reason to not support it.

Google Contact example:
i

@matbalazsi
Copy link

+1

1 similar comment
@k-vekos
Copy link

k-vekos commented May 10, 2018

+1

@mglavall
Copy link

@natcohen Also, there is a bug that when you close the stacked modal, the button that opened it stays focused. It can be seen in your plunkr also: http://plnkr.co/edit/ZGYAbyJXwn8IWqz4sme8?p=preview

@ashgibson
Copy link

+1

@firasomrane
Copy link

@dmitrysteblyuk please where to add this exactly?

modalRef.result.catch(() => {})
    .then(() => {
        if (document.querySelector('body > .modal')) {
            document.body.classList.add('modal-open');
        }
    });

@dmitrysteblyuk
Copy link

dmitrysteblyuk commented Jun 27, 2018

@firou1925 you add it just after opening a modal with “open” method (it returns modalRef).
I don’t know though if this workaround is still working

@nortain
Copy link

nortain commented Jul 9, 2018

I would also like to see a modal in modal support to be added to bootstrap. Confirmation messages inside of modal are hardly a uncommon, or even bad IMO Ux design. Ngb-alerts are too limited to handle this and using a native confirm is just an eye sore compared to a custom built confirmation component. :(
It is nice that @dmitrysteblyuk quick fix does a good job of handling this situation when 2 modals are involved.

@denkerny
Copy link

denkerny commented Jul 24, 2018

@dmitrysteblyuk still works, thx for the answer

@tan9
Copy link

tan9 commented Aug 3, 2018

If you tend not to apply the workaround provided by @dmitrysteblyuk everywhere, register a MutationObserver like:

https://gist.github.com/hyshan/ae791e420bbad1ffa31eafbbd350e290

@benouat
Copy link
Member

benouat commented Aug 29, 2018

Stacked modals will land in 3.2.0

@aldyson
Copy link

aldyson commented Aug 30, 2018

@benouat Will the issue discussed by @Vx2gas and @dmitrysteblyuk be fixed as part of this? i.e. .modal-open being removed after closing a stacked modal, despite a modal still being open?

@benouat
Copy link
Member

benouat commented Aug 30, 2018

@aldyson yes it will! We now have a test for this 😃

it('should not remove "modal-open" class on body when closed modal is not last', async(() => {

@denkerny
Copy link

denkerny commented Nov 2, 2018

@dmitrysteblyuk is there a solution for make parent modal scrolling(>100vh) when child modal is open?

@shrikantk3
Copy link

$('body').on('click','a[data-toggle="modal"]', function(e){
setTimeout(function() {
$('body').addClass('modal-open');
},500);
});
Working perfectly try it.

@wildhart
Copy link

I agree that stacked models have many valid use cases. I've inherited a project which makes extensive use of it. Rather than adding code to every modalRef.result.catch and modalRef.result.then (for modals which are closed instead of dismissed), I have created the following global intercepter for modalRef:

const checkIfOtherModalsOpen = () => {
	setTimeout(() => {
		if (document.querySelector('.modal.show')) {
			document.body.classList.add('modal-open');
		}
	});
}

const origModalOpen = this.modalService.open.bind(this.modalService);

this.modalService.open = (...args) => {
	const modalRef = origModalOpen(...args);
	const origPromise = modalRef.result;
	modalRef.result = new Promise((resolve, reject) => {
		origPromise.then(result => {
			resolve(result);
			checkIfOtherModalsOpen();
		}).catch((error) => {
			reject(error);
			checkIfOtherModalsOpen();
		})
	})
	return modalRef;
}

I've put this in the ngOnInit() in by app.component.ts

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

No branches or pull requests