Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

Conversation

@maxisam
Copy link

@maxisam maxisam commented Nov 17, 2014

To work with bootstrap 3.3.1. Ref #2970

@maxisam
Copy link
Author

maxisam commented Nov 17, 2014

To work with bootstrap 3.3.1. Specifically handle this change

Here is the demo

@chrisirhc
Copy link
Contributor

Thank you!
This also needs an accompanying test. It's also helpful when you're submitting pull requests to refer to the issue you're trying to resolve in the pull request message body. Like this: #2970
So that users will be able to follow where's the latest fix.

@BeMxself
Copy link

It doesn't work well, this code get body's scrollHeight, but sometimes body's scrollHeight is not high enough, please check it.
2014-11-28 21 46 24

@maxisam
Copy link
Author

maxisam commented Nov 28, 2014

Interesting. Can you put it in plunk or somewhere so I can check it ? This is the way how original bootstrap modal does the trick. So if there is an issue with this approach, the original one would have the same issue.

@BeMxself
Copy link

you can open your demo by IE11 to reproduce this bug.

The bootstrap.js is getting ".modal" Element's scrollHeight, not body's scrollHeight.

@jonathaningram
Copy link

If this change mirrors the core BS change, shouldn't it also first set the height to 0?

angularBackgroundDomEl
  .css('height', 0)
  .css('height', body[0].scrollHeight + 'px');

Note: I'm not sure exactly why BS do that, maybe it's to do with reflow or something.

@maxisam
Copy link
Author

maxisam commented Dec 2, 2014

I think @BeMxself point it out correctly. I should get the modal-window element instead of body. I am thinking about how to achieve it easily.

@maxisam
Copy link
Author

maxisam commented Dec 2, 2014

here is the demo to walk around this issue. I think a proper way requires a lot of work. I will try if I can make it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants