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

Inconsistent modal positioning #261

Closed
sergeymorkovkin opened this issue Jan 7, 2019 · 18 comments
Closed

Inconsistent modal positioning #261

sergeymorkovkin opened this issue Jan 7, 2019 · 18 comments
Assignees
Labels
enhancement potential improvement

Comments

@sergeymorkovkin
Copy link

  1. Open modal dialog (non-overflowing, content is overflowing)
  2. Resize window -> modal changes it's position permanently, shifts right

This is caused by the fact that checkScrollbar() results different values when scrollbar is hidden.

@thednp
Copy link
Owner

thednp commented Jan 7, 2019

What do you suggest?

@sergeymorkovkin
Copy link
Author

sergeymorkovkin commented Jan 8, 2019

Reasonable question. I'd recommend remembering the value of bodyIsOverflowing and scrollbarWidth before opening modal dialog. Next thing to do would be checking the most recent code in Bootstrap. It seems to work. And, of course, considering modalIsOverflowing may change while dialog is open.

@thednp
Copy link
Owner

thednp commented Jan 8, 2019

OK @sergeymorkovkin I've had a look at it and the original plugin, so let me give you a suggestion: open bootstrap-native.js and comment lines 1336+1337

        // checkScrollbar();
        // setScrollbar();

Do all the tests you can think of and let me know how it works. I am now considering that perhaps we were over-doing some things.

@sergeymorkovkin
Copy link
Author

That worked perfectly for static modals! Whenever I resize the window, modal always persists it's position. However, when content height changes dynamically and modalIsOverflowing changes, the modal dialog shifts either right (when modal height shrinked) or left (when modal height expanded). Conslusion is we still need these functions.

@thednp
Copy link
Owner

thednp commented Jan 8, 2019

I think if you also comment line 1183 in the adjustDialog() function body, the behavior is pretty much the same as the observing the original plugin.

thednp added a commit that referenced this issue Jan 8, 2019
* fixing #261
* demo updates
thednp added a commit that referenced this issue Jan 8, 2019
* fixing #261
* demo updates
@thednp
Copy link
Owner

thednp commented Jan 8, 2019

@sergeymorkovkin please disregard the previous and download latest master, give it a try, let me know how it goes.

@thednp thednp added the enhancement potential improvement label Jan 8, 2019
@sergeymorkovkin
Copy link
Author

Can't confirm it's good, unfortunately. It possibly works consistently with the original bootstrap modal, but positioning is not correct yet. I've prepared a file with the context I use. It should simplify things for you.
modal-demo.html.zip

@thednp
Copy link
Owner

thednp commented Jan 9, 2019

I think the way of the original plugin looks is a bit off, I think our script does it better slightly, and I am very happy with the result to be honest.

Also please try the upcoming 2.0.25 version (MASTER) not the CDN version 2.0.24.

@sergeymorkovkin
Copy link
Author

Agree. We're at least trying to manage these scrollbars. I've tried version from master and my comment is about it.

@thednp
Copy link
Owner

thednp commented Jan 9, 2019

I don't think the positioning is OFF in any shape or form, it just looks normal in any circumstance.

@sergeymorkovkin
Copy link
Author

sergeymorkovkin commented Jan 9, 2019

Try checking either of those chechboxes and then open modal. Here's how it works in Chrome 71.
This is how the most recent version from master branch works. Video demo.

@thednp
Copy link
Owner

thednp commented Jan 9, 2019

Interestingly I cannot confirm this at all in my testing, it's always consistent, but later on I found you're using this CSS

    .modal-dialog {
      max-width: 100%;
      padding: 0 20px;
    }

UPDATE:

Only use this CSS to do the test, and you'll surely see no alignment issue:

    .form {
      position: absolute;
      z-index: 10000;
    }

    .container {
      background: red;
    }

    .overflow-body,
    .overflow-modal {
      display: none;
    }

    body.body-is-overflowing .overflow-body {
       display: inline-block;
    }

    body.modal-is-overflowing .overflow-modal {
      display: inline-block;
    }

@thednp thednp closed this as completed Jan 9, 2019
@sergeymorkovkin
Copy link
Author

It's masking the problem, not solving. In my use case the dialog should perfectly fit container width from both sides.

@thednp
Copy link
Owner

thednp commented Jan 9, 2019

I will commit new code in a couple minutes.

thednp added a commit that referenced this issue Jan 9, 2019
* Further fixes for #261
* toast-native.js was missing
thednp added a commit that referenced this issue Jan 9, 2019
* Further fixes for #261
* toast-native.js was missing
@thednp
Copy link
Owner

thednp commented Jan 9, 2019

@sergeymorkovkin we good?

@sergeymorkovkin
Copy link
Author

Not really :(

Whenever the "Modal is overflowing" is checked, the modal shifts left by the width of the scrollbar. Other cases seem to be okay with your last version.

@thednp
Copy link
Owner

thednp commented Jan 9, 2019

Well I think it's pointless to go beyond possibility, I think the component behavior is as close to the original plugin as will ever be, I am very happy with the outcome and consider the issue to be truly closed.

Thanks for reporting and best of luck.

@sergeymorkovkin
Copy link
Author

I've tested the most recent version and was able to make it work the way I need by enforcing modal scrollbar. Whoever might need it, please use:

  1. Use BSN 2.0.25 or later
  2. .modal {padding-right: 0 !important;}
  3. .modal-open .modal {overflow-y: scroll !important;}

Thank you for your great support!

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

No branches or pull requests

2 participants