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

Fix padding with modal and fixed navbar #21800

Closed
wants to merge 7 commits into from
Closed

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Jan 20, 2017

Fixes #18373, fixes #21700, fixes #20824, fixes #21840.

Now _setScrollbar:

  • Save the current body padding-right in this._originalBodyPadding
  • Add a pading-right to the body corresponding to the current padding-right + the scrollbar size
  • Save the current padding-right of each fixed element in the element's 'padding-right' data-attribute
  • Add a pading-right to each fixed element corresponding to the current padding-right + the scrollbar size

And _resetScrollbar:

  • Reset the body padding-right to the saved value
  • Reset the padding-right of each each fixed element to the saved value

The PR #17536 added 2 unit tests to verify that the padding-right is properly added to the body. However those 2 tests were ineffective because:

  • The function _getScrollbarWidth relies on the css class .modal-scrollbar-measure to determine the scrollbar width and therefore the value of the padding-right to add to the body. But the class was not present in the unit tests.
  • QUnit doesn't add a scrollbar even when en element is higher than the viewport, the function _getScrollbarWidth was always returning 0px, therefore during a QUnit test the modal plugin was always considering that there is no sidebar, so no padding-right was added to the body

Note: _getScrollbarWidth, is called by _checkScrollbar and sets the current scrollbar width in this._scrollbarWidth. This value is later used by _setScrollbar to determine the width of the padding to add to the body and fixed elements. So _setScrollbar depends on _getScrollbarWidthand .modal-scrollbar-measure.

In order to solve the first issue, this PR add the Bootstrap css to the Unit test so the class .modal-scrollbar-measure and _getScrollbarWidth can work as intended during the unit tests.

To solve the second one, the tests have been modified to simulate a scrollbar by adding 10px to the html element. Then after the modal is opened we verify that a padding-right of 10px (corresponding to the width of the simulated scrollbar) has been added to the body element.
Note: The simulated scrollbar is added to the html element, and after the modal is opened the test verify the padding-right of the body (html != body).

In addition the tests now also check for the padding-right to be applied on the fixed elements.

@Johann-S
Copy link
Member

Johann-S commented Apr 2, 2017

Closed due to the merge of : #18441

@Johann-S Johann-S closed this Apr 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants