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 background does not respect non statically positioned elements #12

Closed
gjrevans opened this issue Jul 17, 2019 · 6 comments
Closed

Comments

@gjrevans
Copy link

Unfortunately, due to the nature of z-index the modal element will not respect non statically positioned elements that occur after the modal in the DOM. As a result, the transparent modal background will not sit on top of all content.

To correct, I would suggest extending the open function to append a transparent background at the end of the html body (as bootstrap does) instead of containing it within the context of the modal block. Subsequently, the close function should remove it. The modal body itself should be given a z-index greater than that of the transparent modal background.

For reference please see attache screenshot.

As you can see, the form elements wrapped in a form-group class which has been assigned position: relative. Will sit on top of the modal background.

Screenshot 2019-07-17 14 04 37

@excid3
Copy link
Owner

excid3 commented Jul 18, 2019

Hey @gjrevans can you post a codepen or something showcasing the issue?

I think the appending of the background makes sense and is probably the way to do it, but we may also have to dynamically generate the modal down there too, which would be not a simple change.

@gjrevans
Copy link
Author

Hey @excid3,

Here's a codepen showcasing the issue: https://codepen.io/gjrevans/pen/GVRYMm?editors=0010.

You shouldn't need to dynamically generate the modal. Just ensure the modal's position is set to something other than static (so that it respects z-index) and give it a z-index higher than the modal background which will get dynamically appended at the end of the body.

P.S. A few friends and I ran into this issue while using Jumpstart package you created. We grabbed the pro license as we wanted to give Stimulus / Tailwind a go. Thanks for putting that solution together, it's great!

@gjrevans
Copy link
Author

gjrevans commented Jul 18, 2019

Here's a very rudimentary working version with the suggested improvement: https://codepen.io/gjrevans/pen/gVOqxW

I've moved the modal background out of the HTML side of things, created a local variable in the modal controller for modalBackground and then append & remove it using the open & close actions. I've also updated the closeBackground function to instead closeContainer as the background was no longer necessary.

The modal background has a z-index: 40 and the modal container a z-index: 50 (and is position: fixed and thus it and it's children will respect z-index). Arguably it would be better to assign them much higher z-index values like 1000 and 1010 to ensure they are alway on top of everything -- this could be done inline to avoid creating new classes as you've already done with the transparent background.

@excid3
Copy link
Owner

excid3 commented Jul 19, 2019

Fantastic, thanks so much. This will be good to have fixed. 👍

I agree, we may want to use like an inline z-index specified above the max that Tailwind offers, that way we can make sure it's above any other z-50 items on the page. This may not be an issue, if the z-50 of the background takes precedence since it's the very last item in the DOM. Plus people may customize Tailwind to add like a z-1000 and we'd want to handle that case too ideally.

@excid3
Copy link
Owner

excid3 commented Jul 19, 2019

Thanks! This is fantastic.

Last night I went and updated everything to work similar to your codepen. I made a few enhancements to allow customizing the backdrop and such.

Just released it as 1.0.0, so hopefully everything is good. This is backwards incompatible because of the HTML changes, hence the major version bump.

@excid3 excid3 closed this as completed Jul 19, 2019
@gjrevans
Copy link
Author

@excid3 that’s awesome! 👏 Happy it helped, thanks for putting that through so quickly!

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