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: Apply any preexisting body padding again after closing #15930

Merged
merged 1 commit into from Feb 26, 2015

Conversation

@hnrch02
Copy link
Collaborator

commented Feb 26, 2015

Modifies #15273 to ignore padding set via CSS.

@hnrch02 hnrch02 added the js label Feb 26, 2015
@hnrch02 hnrch02 added this to the v3.3.4 milestone Feb 26, 2015
@hnrch02 hnrch02 force-pushed the modal-restore-inline-padding branch 2 times, most recently Feb 26, 2015
@cvrebert
cvrebert reviewed Feb 26, 2015
View changes
js/modal.js Outdated
@@ -259,11 +260,12 @@

Modal.prototype.setScrollbar = function () {
var bodyPad = parseInt((this.$body.css('padding-right') || 0), 10)
this.originalBodyPad = this.$body.attr('style') ? bodyPad : null

This comment has been minimized.

Copy link
@cvrebert

cvrebert Feb 26, 2015

Member

Won't this break with something like:

body { padding-right: 5px; }
<body style="color: red;">

?

This comment has been minimized.

Copy link
@hnrch02

hnrch02 Feb 26, 2015

Author Collaborator

Yup, that's why I haven't merged it yet. Was thinking about checking it with /padding-right/i, what do you think?

This comment has been minimized.

Copy link
@cvrebert

cvrebert Feb 26, 2015

Member

What about !!this.$body[0].style.paddingRight ?

This comment has been minimized.

Copy link
@hnrch02

hnrch02 Feb 26, 2015

Author Collaborator

Much better - I keep forgetting that Element.style does not include computed styles.

@hnrch02 hnrch02 force-pushed the modal-restore-inline-padding branch to 039ba6d Feb 26, 2015
@hnrch02

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

@cvrebert LGTY now? Added another unit test for the case you described.

@cvrebert

This comment has been minimized.

Copy link
Member

commented Feb 26, 2015

What about <body style="padding-right: 5%">? I think the current code would replace 5% with its calculated pixel value when doing the restoration.

@hnrch02 hnrch02 force-pushed the modal-restore-inline-padding branch from 039ba6d to 6656f8e Feb 26, 2015
@hnrch02

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Good catch. If only I was as wise as you are :bowtie:

@cvrebert

This comment has been minimized.

Copy link
Member

commented Feb 26, 2015

What would programming be without edge cases? :-P

js/modal.js Outdated
@@ -259,11 +260,12 @@

Modal.prototype.setScrollbar = function () {
var bodyPad = parseInt((this.$body.css('padding-right') || 0), 10)
this.originalBodyPad = document.body.style.paddingRight || null

This comment has been minimized.

Copy link
@cvrebert

cvrebert Feb 26, 2015

Member

What about || '' instead of || null, and then getting rid of the ternary below?

This comment has been minimized.

Copy link
@hnrch02

hnrch02 Feb 26, 2015

Author Collaborator

I'm getting a little tired, that I could have noticed.

@hnrch02

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Nothing of interest, certainly 😄

@hnrch02 hnrch02 force-pushed the modal-restore-inline-padding branch from 6656f8e to 3a01490 Feb 26, 2015
.bootstrapModal('show')
})

QUnit.test('should ignore values set via CSS when trying to restore body padding after closing', function (assert) {

This comment has been minimized.

Copy link
@cvrebert

cvrebert Feb 26, 2015

Member

This test description needs to be updated

@cvrebert

This comment has been minimized.

Copy link
Member

commented Feb 26, 2015

Once that description is fixed, 👍 💯 :shipit:

@hnrch02 hnrch02 force-pushed the modal-restore-inline-padding branch from 3a01490 to 142a9e4 Feb 26, 2015
hnrch02 added a commit that referenced this pull request Feb 26, 2015
Modal: Apply any preexisting body padding again after closing
@hnrch02 hnrch02 merged commit 442d2dd into master Feb 26, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@hnrch02 hnrch02 deleted the modal-restore-inline-padding branch Feb 26, 2015
@cvrebert cvrebert referenced this pull request Feb 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.