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 script adds margin-right and padding-right for .sticky-top #27071

Closed
Gnevyshev opened this issue Aug 15, 2018 · 16 comments · Fixed by #30621
Closed

Modal script adds margin-right and padding-right for .sticky-top #27071

Gnevyshev opened this issue Aug 15, 2018 · 16 comments · Fixed by #30621

Comments

@Gnevyshev
Copy link

Windows 7
Firefox 61.0.2

Why modal script adds margin-right and padding-right for .sticky-top?

qip shot - screen 2018 08 15 10-16-37

@Johann-S
Copy link
Member

We just add padding-right to the body to center horizontally our moral when there is no scrollbar

@Johann-S
Copy link
Member

Ok I made this CodePen to demonstrate the issue: https://codepen.io/Johann-S/pen/KxKpOR

@VDWWD
Copy link

VDWWD commented Aug 20, 2018

Fixed it with this

<script type="text/javascript">

    $('body').on('show.bs.modal', function () {
        $('.sticky-top').css('margin-left', '-=0px');
    });

    $('body').on('hidden.bs.modal', function () {
        $('.sticky-top').css('margin-left', 'auto');
    });

</script>

@Johann-S
Copy link
Member

@mdo I'm a bit lost here, I understand the issue, I know how to fix it but I'm not sure what I'll do is the right fix.

My fix will not add margin-right or padding-right if the element have our container class (.container), but I'm not sure if it's the right things to do 🤔

My fix replace those lines :
https://github.com/twbs/bootstrap/blob/v4-dev/js/src/modal.js#L432-L433

By:

        const fixedContent = [].slice.call(document.querySelectorAll(Selector.FIXED_CONTENT))
          .filter((elem) => !elem.classList.contains('container'))
        const stickyContent = [].slice.call(document.querySelectorAll(Selector.STICKY_CONTENT))
          .filter((elem) => !elem.classList.contains('container'))

/CC @XhmikosR if you have any ideas, @zalog because you worked on that too

@Gnevyshev
Copy link
Author

Now you can see the problem on the site:
http://agnevy6d.beget.tech
Click "ЗАКАЗАТЬ ЗВОНОК" under phone numbers.

Pay attention to file "main.css", line 38.
Empirically, I realized that setting these properties solves the problem.
If uncomment line 38 (and reload the page), there will be no problem.

@XhmikosR
Copy link
Member

I too see the issue for sure. @Johann-S's fix seems OK, do we have a PR for this?

@Johann-S
Copy link
Member

Nope because I'm not sure my fix is correct and won't create side effects

@SharakPL
Copy link

SharakPL commented Jan 29, 2019

Why do you even go for margin and padding? body gets padding-right when modal is activated so everything else should be contained within the body. With v.4.2.1 when body gets class modal-open and style i.e. padding-right: 17px added, fixed elements get extra padding-right and sticky elements get extra padding-right and negative margin-right. That simply looks bad:

stickybug

FIX:
Both padding and margin should be left as they are!

Sticky element will be contained by the parent's width so nothing needs changing here (only when browser doesn't do sticky).
Fixed element is contained by the viewport's width, but you can position it properly with right: 17px.
To be precise it's right: <whatever is added to body as padding-right>.

https://codepen.io/SharakPL/pen/OdRayP

Lausselloic added a commit to Orange-OpenSource/Orange-Boosted-Bootstrap that referenced this issue Feb 26, 2019
… + add position-fixed to fixed selector, remove sticky-top from fixed content, and finish by removing all sticky content selector as they're useless see : twbs/bootstrap#27071
@Ana06
Copy link

Ana06 commented Apr 23, 2019

any update?

@cosmo-101
Copy link

This is still not fixed so this is my solution:

$('body').on('show.bs.modal', function () { $('.sticky-top').addClass("fixModal"); }); $('body').on('hidden.bs.modal', function () { $('.sticky-top').removeClass("fixModal"); });

And the css class .fixModal

.fixModal {
    padding-right: 0 !important;
    margin-right: 0 !important;
}

@cosmo-101
Copy link

cosmo-101 commented Aug 27, 2019

@XhmikosR I still have problems even with home solution with this issue. It will be solved on the next release?

@XhmikosR
Copy link
Member

As you can see, the issue is still open and it even has the label help wanted added, so I'd say no :P

Any PRs which might fix this are welcome as usual :)

@Seth10001
Copy link

I just ran into this problem when migrating from v3 to v4. Looks like it might have been fixed in #30621, but it's still waiting on a fix for a test conflict issue

@utilmind
Copy link

utilmind commented Jul 24, 2020

Having the same issue. Steps to reproduce in my web app:

  1. Open any modal dialog with sticky panel inside.
  2. This dialog have a button, that opens another dialog (it temporary hide the first dialog in bs.shown event)
  3. Close the second dialog (first dialog will be shown again)
  4. Close the first dialog.
  5. Re-open the first dialog. Now sticky panel have extra "margin-right: -17px;"

UPD. I have fixed my problem applying patch by @muhamadamin1992 1c9f4a4

        $(stickyContent).each(function (index, element) {
          if (window.innerWidth <= element.clientWidth + _this10._scrollbarWidth) { // this is added line
            var actualMargin = element.style.marginRight;
            var calculatedMargin = $(element).css('margin-right');
            $(element).data('margin-right', actualMargin).css('margin-right', parseFloat(calculatedMargin) - _this10._scrollbarWidth + "px");
          }
        }); // Adjust body padding

@rob101
Copy link

rob101 commented Feb 9, 2022

This issue unfortunately persists if you are using offcanvas menu in BS5.

@XhmikosR
Copy link
Member

XhmikosR commented Feb 9, 2022

@rob101 please make a new issue with all the needed info.

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

Successfully merging a pull request may close this issue.

10 participants