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

Support for stacking modals #95

Closed
wants to merge 2 commits into from

Conversation

antstorm
Copy link
Contributor

Do not merge this yet!

Ok, this is not something I really like, but some people think that stacking modals is not that bad. One of those people being the designer in the company I work for :)

I really like Remodal and wanna continue using it, but for this I need stacking modals on top of each other. I've made some initial changes to the library in order to support that.

Basically changes include:

  • Overlay div is moved inside the wrapper, so that every modal has it's own overlay
  • Instead of a global remodal-is-active class, I'm using remodal-is-opened class on a wrapper, that allows showing/hiding individual modals
  • There's a stack option in settings that determines wether the modal should replace the current one of stack on top of it. Default is false in order to keep original behaviour

So I know that it kinda goes against your original idea, but this is quite a minimal change that won't fight people trying to implement this behaviour.

This is still work in progress, just wanted to make sure you are happy with the direction this is going. Otherwise I'll have to maintain a version of Remodal, that supports stacking, which is something I'm not sure I wanna do (keeping two versions of the same library).

In case you're happy to proceed I'll do some refactoring, add usage examples and update docs.

@antstorm antstorm mentioned this pull request Mar 16, 2015
@AlecRust
Copy link
Contributor

Fantastic! Really needed this imo. 👍

@vodkabears
Copy link
Owner

👍

@kohaistyle
Copy link

Truly needed ! Stackable modals for confirm/yes-no windows, for exmaple

@vodkabears
Copy link
Owner

I develop the new version - 1.0.0(in the 'dev' branch). So, i don't accept any PR until v1.0.0 will be released. Sorry.

@vodkabears
Copy link
Owner

With API of the v1.0.0 it will be possibe to make this in minor versions without incompatibilities.

@PoeHaH
Copy link

PoeHaH commented Jun 20, 2015

Shouldn't there be a line of code that enlarges the z-Index on the wrapper? How can this work with always the same z-index?

@antstorm
Copy link
Contributor Author

@PoeHaH correct me if I'm wrong, but elements with the same z-index stack on top each other, latest to appear in HTML to be on the top.

@antstorm
Copy link
Contributor Author

@vodkabears since the 1.0.0 is out, I'm gonna resurrect this PR. Or maybe open another one, since this one's not gonna merge very well.

@vodkabears
Copy link
Owner

Cool. Create another one, pls, i'll close it.

@vodkabears vodkabears closed this Jun 20, 2015
@PoeHaH
Copy link

PoeHaH commented Jun 21, 2015

@antstorm i also thought that, but it wasn't working for me so I added incremental zIndex, which fixed my issues.

@antstorm
Copy link
Contributor Author

@PoeHaH good to know that! I'll definitely take this into account.

@whwnow
Copy link

whwnow commented Jul 1, 2015

it has released 1.0.1 now . waiting this ! @antstorm

@antstorm
Copy link
Contributor Author

Here's the new PR — #162. Let's get done and it merged.

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.

None yet

6 participants