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

Overlay disappears when starting a modal from another modal #30

Closed
HelloGravity opened this issue Nov 7, 2015 · 35 comments
Closed

Overlay disappears when starting a modal from another modal #30

HelloGravity opened this issue Nov 7, 2015 · 35 comments

Comments

@HelloGravity
Copy link
Contributor

Here is a jsfiddle that demonstrates the bug: http://jsfiddle.net/dmt0z2rx/
With jQuery and bootstrap's original javascript it works: http://jsfiddle.net/qkc6bLha/
The bug happens because when the first modal closes, it removes the overlay after a short delay, and the second modal uses that same overlay element, which gets removed.
I fixed the bug in my fork by using a separate overlay element for each modal: HelloGravity@f84a093

@thednp
Copy link
Owner

thednp commented Nov 7, 2015

Thanks for reporting. I think creating multiple overlays is not the best solution, so I'm gonna investigate this issue and fix it.

@HelloGravity
Copy link
Contributor Author

I think that not using multiple overlays will not be a good solution. Here is the same example from before with bootstrap's original javascript, but without closing the first modal before opening the second: http://jsfiddle.net/sev5ux8g/ - they use 2 overlays for it, and it makes the background even darker. This won't happen with only one overlay.

@thednp
Copy link
Owner

thednp commented Nov 7, 2015

That's why I'm saying, no multiple overlays, but only one and properly used.

On Sat, Nov 7, 2015 at 6:29 PM, Yaron [email protected] wrote:

I think that not using multiple overlays will not be a good solution. Here
is the same example from before with bootstrap's original javascript, but
without closing the first modal before opening the second:
http://jsfiddle.net/sev5ux8g/ - they use 2 overlays for it, and it makes
the background even darker. This won't happen with only one overlay.


Reply to this email directly or view it on GitHub
#30 (comment)
.

@HelloGravity
Copy link
Contributor Author

I think that a darker background is better, since it emphasizes that a modal was added.
Isn't this project about providing the same functionality as the original bootstrap js?
I think that it shouldn't changes the behavior from what was intended in bootstrap.

@thednp
Copy link
Owner

thednp commented Nov 7, 2015

The project is about making it lighter and if possible, better.

On Sat, Nov 7, 2015 at 7:20 PM, Yaron [email protected] wrote:

I think that a darker background is better, since it emphasizes that a
modal was added.
Isn't this project about providing the same functionality as the original
bootstrap js?
I think that it shouldn't changes the behavior from what was intended in
bootstrap.


Reply to this email directly or view it on GitHub
#30 (comment)
.

@thednp
Copy link
Owner

thednp commented Nov 8, 2015

OK back to my PC. Here's the plan:

  • on init, the Modal will create a backdrop to be used for all instances and it will check if (backdrop !== null) {} before that
  • all Modal instances will access this backdrop and the script makes sure to properly handle events

@thednp
Copy link
Owner

thednp commented Nov 9, 2015

The thing that came to my mind is, why use nested modals? That's bad design practice to me.

@ghost
Copy link

ghost commented Nov 9, 2015

I can think of a few use cases where modals on top of modals is not only valid but good design practices. For example if you have a dashboard that shows you aggregated numbers in a summary view. You can then click on any of those numbers to see things in more detail. Let's say that detail view opens up in a modal

From the detail view let's also say that you are viewing orders for an order management system. You can then click any order number to view some additional details like shipping address or tracking information from a delivery company

@thednp
Copy link
Owner

thednp commented Nov 9, 2015

@osdlge in that case, the best would be to update the modal content/template, instead of opening a nested modal, simply open a popover, or a tooltip. Seems more simple and feels like better design. Nesting is, IMO, only for page/content navigation menus. Having nested tabs, modals, carousels, collapses, is a bad practice and even if it is to be made possible, it would require alot of redundant code, IMO.

@HelloGravity
Copy link
Contributor Author

I found this project when looking for a way to use bootstrap without
jquery, since it is heavy and I don't use it.

My original code and design had a modal with options for the user to choose
from, and the modal disappeared after receiving the user's choice, and if
an error occures another modal would popup asking the user what to do about
it. I did not want 2 modals stacked, I wanted a modal to replace the
previous modal.

I am now porting my code from bootstrap+jquery to bootstrap.native, since I
expected the behaviour to be the same, while I get a lighter website - and
I think this is the case for many people using this library - wanting to
get the exact same behaviour, just without jquery. I can't rely on
bootstrap.native if the project aims to change the behaviour of bootstrap,
and not giving me exactly the same.
On Nov 9, 2015 9:02 PM, "thednp" [email protected] wrote:

@osdlge https://github.com/osdlge in that case, the best would be to
update the modal content/template, maybe some collapses or perhaps tabs.


Reply to this email directly or view it on GitHub
#30 (comment)
.

@ghost
Copy link

ghost commented Nov 9, 2015

@thednp that kind of defeats the purpose of having a smooth UI, I use a modal to display a much smaller popup on the current modal with a nice look and feel to give the user access to just the small set of data they want to see. Switching the users context wouldn't be as clean or efficient as having them close the modal to be back to where they left off

@thednp
Copy link
Owner

thednp commented Nov 9, 2015

@osdlge you can create a custom handler to your modals' trigger buttons and content in a way that you don't have to create nested modals. Here's how I would write it:

// cache the current title and content of the parent modal in an object
var parentModal = {};
parentModal.title = myModalTitle.innerHTML;
parentModal.body = myModalBody.innerHTML;
parentModal.footer = myModalFooter.innerHTML;

// this is a button inside the parent Modal, when clicked will replace the content of the Modal
buttonToChildModal.addEventListener('click', function(){
  // add your CHILD Modal title, content and footer to the Modal
  // while this new content should have a button to "link back", it's handler is just below
}, false);

// this is a button inside the same Modal but with the child's content, when clicked will replace the content of the Modal with the cached values
buttonToParentModal.addEventListener('click', function(){
  // add your PARENT Modal title, content and footer back to the original Modal
}, false);

Note that this code is not tested, will not work on the fly, it's just to give you an idea.

@HelloGravity in most cases (I mean for the most people and their most projects) there is no need for the exact same behavior, as explained in the demo, for each particular script example, you get a similar basic behavior. For instance dropdown: you click it and it opens the dropdown, or modal you click a button and opens a pop-up modal and dismiss on any target with reference to that specific modal.

As I understand, there are some issues with Modal, I am committed to fix them all, no problem, but I'm not sure about this nesting of components, I always avoid it to keep the code simple, maintainable and usable. I really tried to do nesting for some components, but I figured the code went far more where I don't really need it. For most particular cases, the best thing to do is NOT to change the source of the script if possible, but find a solution that's best local use, as shown above.

@thednp thednp closed this as completed Nov 9, 2015
@thednp
Copy link
Owner

thednp commented Nov 9, 2015

Oops closed by mistake.

@thednp thednp reopened this Nov 9, 2015
@thednp
Copy link
Owner

thednp commented Nov 11, 2015

QUESTION:

Do you want me to write a tutorial in the demo documentation on how to toggle modal content instead of using nested modals?

Let me know what you think, when I get some time I'll get to fixing the issues and update the doc.

@ghost
Copy link

ghost commented Nov 11, 2015

If you could do up a quick example I think that would help, I'm having a hard time trying to figure out the pseudo code you provided in #30 (comment)

@thednp
Copy link
Owner

thednp commented Nov 11, 2015

Alright, then rest assured, within a few days, I think I will get to it, you can work on something else meanwhile.

@thednp
Copy link
Owner

thednp commented Dec 2, 2015

Sorry I didn't have the time to stick to the issue, having other stuff to do, still, have you managed to figure it out? Meanwhile, I'll try to fix the current issues.

@thednp
Copy link
Owner

thednp commented Dec 2, 2015

I will try to work on the issue as @HelloGravity put it, I think his point worth taking into account and his example is another idea of what I was thinking for you @osdlge, I will let you guys know asap, ok?

@thednp
Copy link
Owner

thednp commented Dec 2, 2015

@HelloGravity please check my fork here, this should fix your issue.
http://jsfiddle.net/kjvrghmz/

The Modal code shown in the above link is part of the final working code.

thednp added a commit that referenced this issue Dec 3, 2015
* fixed the issue [#30](#30)
* added missing functionality for overflow adjustments like the original plugin
* major change to the prototype structure, for easier function binding
@thednp
Copy link
Owner

thednp commented Dec 3, 2015

Fixed in 7985fae

@thednp thednp closed this as completed Dec 3, 2015
@thednp
Copy link
Owner

thednp commented Dec 3, 2015

@HelloGravity an even better example for you, now with latest version from CDN

http://jsfiddle.net/pzmk50tf/

@thednp
Copy link
Owner

thednp commented Mar 31, 2016

I've implemented into the code. See example.

@ghost
Copy link

ghost commented Mar 31, 2016

@thednp can you take a look at this example:
http://jsfiddle.net/pzmk50tf/2/

This is what I am using locally and I prefer how it works because it allows modal stacking

@thednp
Copy link
Owner

thednp commented Mar 31, 2016

@osdlge you should check the latest code in the master repo, see the new updated demo here.

@ghost
Copy link

ghost commented Mar 31, 2016

@thednp your demo is the one I modified with my changes. My challenge is I don't want the initial modal to close

Imagine if the first modal was a listing of all orders for a specific customer. It was just a table with columns like
order id cost shipment date shipping company (etc etc)

clicking on any order id would open a second modal showing the products associated with that order

It doesn't make sense (to me) to remove the user from the context of the list of orders that they were in just to show the specific details of one order id

I totally get that this is your code and I really appreciate the lack of jquery's size in my project now but I think we may be looking at UI conventions a little differently

@thednp
Copy link
Owner

thednp commented Mar 31, 2016

Yes, some other user pointed out that the original plugin does what my script currently does and didn't before this last commit, so I think you need a fork for your need as well.

@ghost
Copy link

ghost commented Mar 31, 2016

I can definitely fork and submit a PR if you'd accept it. From my own experiences I think this is a better convention but I can see your side of it as well

@thednp
Copy link
Owner

thednp commented Mar 31, 2016

Don't get me wrong: fork it and solve your issue, I seem to have to stick to the original plugin functionality for now.

As a side note: I wish the script to be light, usable and performance driven. If it's a light code can be easily extended for any need such as yours, so take my 2cents as an advice of good coding practice.

@ghost
Copy link

ghost commented Mar 31, 2016

my change is considerably lighter than yours. My change only involved 3 or 4 lines of simple code to detect if any .modal.in elements exist before deciding if to remove the overlay

@ghost
Copy link

ghost commented Mar 31, 2016

your demo
http://jsfiddle.net/pzmk50tf/1/
my demo
http://jsfiddle.net/pzmk50tf/2

if you want to compare side by side

@thednp
Copy link
Owner

thednp commented Mar 31, 2016

Yes I know that. But the original plugin doesn't work like that. I don't know, I think I will leave it as is for now. I have a ton of other stuff to do.

@ghost
Copy link

ghost commented Mar 31, 2016

that's also a different use case. I think this issue should really be split

My use case is I want modal stacking so I can trigger modals on top of modals without losing the overlay

This issue may be specific to switching what modal is in view without losing the overlay

anyway it's your code that I am happy to use and my fork of it works for what I want to do which is great. thanks again for your hard work on this one

@thednp
Copy link
Owner

thednp commented Mar 31, 2016

Thank you, appreciated.

@ghost
Copy link

ghost commented Mar 31, 2016

@thednp
Copy link
Owner

thednp commented Mar 31, 2016

Thank you, I know that too :)

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

No branches or pull requests

2 participants