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.

Sign up for GitHub

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

Jump to bottom

Fix sticky margin when a modal is opened #23669

Merged
merged 4 commits into from
Aug 25, 2017
Merged

Fix sticky margin when a modal is opened #23669

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9d073b
Adjust margin for sticky elements on modal
TechDavid Aug 25, 2017
93c123e
Add unit tests for margin of sticky elements on modal
TechDavid Aug 25, 2017
12e822d
Merge branch 'v4-dev' into fix-sticky-margin
TechDavid Aug 25, 2017
aaaa6a2
Use a template string instead of adding to the first selection
TechDavid Aug 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions js/src/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ const Modal = (($) => {
DATA_TOGGLE : '[data-toggle="modal"]',
DATA_DISMISS : '[data-dismiss="modal"]',
FIXED_CONTENT : '.fixed-top, .fixed-bottom, .is-fixed, .sticky-top',
STICKY_CONTENT : '.sticky-top',
Copy link
Member

@Johann-S Johann-S Aug 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.sticky-top is in FIXED_CONTENT so I don't see the interest to add a new constant here 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because sticky elements also need the padding which was and still is applied to all fixed elements, in addition to the new margin, to have the correct position. However, I could change the references to STICKY_CONTENT to the string .sticky-top if you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that's fine I just wanted to understand your thinking 👍☺️

NAVBAR_TOGGLER : '.navbar-toggler'
}

Expand Down Expand Up @@ -441,6 +442,13 @@ const Modal = (($) => {
$(element).data('padding-right', actualPadding).css('padding-right', `${parseFloat(calculatedPadding) + this._scrollbarWidth}px`)
})

// Adjust sticky content margin
$(Selector.STICKY_CONTENT).each((index, element) => {
const actualMargin = $(element)[0].style.marginRight
const calculatedMargin = $(element).css('margin-right')
$(element).data('margin-right', actualMargin).css('margin-right', `${parseFloat(calculatedMargin) - this._scrollbarWidth}px`)
})

// Adjust navbar-toggler margin
$(Selector.NAVBAR_TOGGLER).each((index, element) => {
const actualMargin = $(element)[0].style.marginRight
Expand All @@ -464,8 +472,8 @@ const Modal = (($) => {
}
})

// Restore navbar-toggler margin
$(Selector.NAVBAR_TOGGLER).each((index, element) => {
// Restore sticky content and navbar-toggler margin
$(Selector.STICKY_CONTENT).add(Selector.NAVBAR_TOGGLER).each((index, element) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe here you should do something like that :

$(`${Selector.STICKY_CONTENT} ${Selector.NAVBAR_TOGGLER}`)

instead of .add

const margin = $(element).data('margin-right')
if (typeof margin !== 'undefined') {
$(element).css('margin-right', margin).removeData('margin-right')
Expand Down
42 changes: 42 additions & 0 deletions js/tests/unit/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,48 @@ $(function () {
.bootstrapModal('show')
})

QUnit.test('should adjust the inline margin of sticky elements when opening and restore when closing', function (assert) {
assert.expect(2)
var done = assert.async()
var $element = $('<div class="sticky-top"></div>').appendTo('#qunit-fixture')
var originalPadding = $element.css('margin-right')

$('<div id="modal-test"/>')
.on('hidden.bs.modal', function () {
var currentPadding = $element.css('margin-right')
assert.strictEqual(currentPadding, originalPadding, 'sticky element margin should be reset after closing')
$element.remove()
done()
})
.on('shown.bs.modal', function () {
var expectedPadding = parseFloat(originalPadding) - $(this).getScrollbarWidth() + 'px'
var currentPadding = $element.css('margin-right')
assert.strictEqual(currentPadding, expectedPadding, 'sticky element margin should be adjusted while opening')
$(this).bootstrapModal('hide')
})
.bootstrapModal('show')
})

QUnit.test('should store the original margin of sticky elements in data-margin-right before showing', function (assert) {
assert.expect(2)
var done = assert.async()
var $element = $('<div class="sticky-top"></div>').appendTo('#qunit-fixture')
var originalPadding = '0px'
$element.css('margin-right', originalPadding)

$('<div id="modal-test"/>')
.on('hidden.bs.modal', function () {
assert.strictEqual(typeof $element.data('margin-right'), 'undefined', 'data-margin-right should be cleared after closing')
$element.remove()
done()
})
.on('shown.bs.modal', function () {
assert.strictEqual($element.data('margin-right'), originalPadding, 'original sticky element margin should be stored in data-margin-right')
$(this).bootstrapModal('hide')
})
.bootstrapModal('show')
})

QUnit.test('should adjust the inline margin of the navbar-toggler when opening and restore when closing', function (assert) {
assert.expect(2)
var done = assert.async()
Expand Down