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

feat(modal): added bootstrap3 support #879

Closed

Conversation

hallister
Copy link

Updated for the new branch.

I added the modal-open class when a modal opens and remove it when it closes, but this may open a can of worms. In Bootstrap 3 they decided to add a modals scrollbar to the modal itself, rather than modal-body.

This is causing a 15px shift in the rest of the body, because the modal scrollbar is absolutely positioned (whereas the body's is not). This is still being worked on per twbs/bootstrap#9855. The last response to this suggests to progmatically determine how wide the scrollbar is on that particular browser, than add a margin-right to the body to offset the difference between the scrollbars. The only way I'm aware of how to do this is to make a hidden element, add it to the dom and get the difference between the element.clientWidth and the element.offsetWidth. Totally worth it to have the scrollbars on the body rather than modal-body, right? 😠

The other issue we may run into, that I haven't tested, is that stacked modals may have multiple scrollbars. So if we have 2 modals in the stack, 2 scrollbars may show. I haven't really tested this so maybe the absolute positioning of the modals will render that moot (the scrollbar of the top modal may overlay the modal below it... hopefully).

@adrianofoschi
Copy link

anyone can post a modal example code that works with bootstrap 3 css and bootstrap3_bis directives?

@mvhecke
Copy link
Contributor

mvhecke commented Aug 30, 2013

@hadokee http://plnkr.co/edit/pkQNcC?p=preview

@hallister
Copy link
Author

It looks like a fix was implemented in bootstrap master: www.github.com/twbs/bootstrap/commit/bae7efbd73484ee0f1a068fcca7fcb6b4791c0c4

I briefly tested it it Chromium and the shift-bug is gone. Assuming it stays this way it appears it will be a CSS-only fix.

@pkozlowski-opensource
Copy link
Member

@hall5714 Thnx for the update. I'm going to review and merge it over the weekend.

@ProLoser
Copy link
Member

ProLoser commented Sep 7, 2013

@pkozlowski-opensource is there no longer any way to do this purely from the HTML? Is there any plans to add this ability back?

@pkozlowski-opensource
Copy link
Member

@ProLoser I'm planning to work on this over the weekend... I got delayed since AngularJS 1.2RC1 / RC2 has some non-backward compatible changes which is a real pain in a neck...

@pkozlowski-opensource
Copy link
Member

@hall5714 @ProLoser I've rebased things on top of the current master (into a new branch bootstrap3_bis2 ... grr, need to find a better way of handling those rebases...) and landed changes for bootstrap3 as d870f21.

It seems to be working OK apart from flickering of scroll bars. Can't spend more time on this today so if someone could help with this it would be great.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants