Skip to content
This repository

Update page padding condition #4223

Closed
pavliy opened this Issue April 27, 2012 · 6 comments

6 participants

Eugene Jason Scott Jasper de Groot Ghislain Seguin Scott Jehl Todd Parker
Eugene

There is a property for fixed toolbars: updatePagePadding.
But in case of pageshow event function updatePagePadding is always being called, event when updatePagePadding is false.

.bind("pageshow", function () {
    self.updatePagePadding(); //Seems to be incorrect because of ignoring updatePagePadding property.
    if (o.updatePagePadding) {
        $(window).bind("throttledresize." + self.widgetName, function () {
             self.updatePagePadding();
        });
    }
})
Maurice Gottlieb MauriceG referenced this issue from a commit in MauriceG/jquery-mobile April 27, 2012
Maurice Gottlieb Propose fix for #4223 - call updatePagePadding() just if updatePagePa…
…dding is enabled
c1c3c78
Ghislain Seguin
Collaborator
gseguin commented May 02, 2012

@scottjehl can you weigh in?
I have the feeling that this is on purpose so on pageshow we do adjust the padding no matter what and update it on page resize only when the option is set.

Jasper de Groot
Owner

@gseguin

Seems to me that the option to set updatePagePadding to false becomes useless when it only sometimes applies.
This would mean that when a page is shown using a transition there is no padding, but when the user refreshes the page or enters the site on that page, there will be padding.

I suggest to pull @MauriceG his PR that fixes this.

@scottjehl can you confirm?

Scott Jehl

Hi all, sorry for the delay in chiming in.

I was reading through the commit for this line (72b0d46) and there isn't any reason mentioned on why that line is outside of the if statement, so that's not terribly helpful...

But if I remember correctly, the option was not intended to disable the dynamic padding, but merely to disable it from updating on resize, for whatever that'd save in performance. I guess the thinking was that the logic didn't need to keep running on resize if the header didn't have content that changed in height.

Moving the line inside the if statement would make the feature more like a setPagePadding boolean rather than an update one. I'm not sure if that's what we want, but I'm open to either way you guys think makes sense. Basically, I added the option to throttle the amount of code executing on resize is all.

Jasper de Groot
Owner

At first I thought the idea behind the option to set this to false, was to make it possible to set padding yourself and don't have it overridden by the framework. For instance when you want content on the exact same height no matter the height of the header. Scott's explanation about it being an "update" and not a "set" function makes sense though.

The padding is already set in the css (see also related issue #4412). Changing it to a "setPagePadding" function requires an explanation in the docs that you have to override those rules with custom css if you set it to false.
If anyone wants this to be changed we need a PR that cover all those aspects.

I am about to close this issue and @MauriceG his PR, since it turned out not to be a bug, but intentionally. We need to edit the info about this option in the docs data attribute reference so it's clear what it is about.

Any objections?

Jasper de Groot
Owner

@toddparker - What is your opinion about this?

Todd Parker

Based on @scottjehl's explanation, I think the way it works now makes sense but the documentation needs to be updated to explain this better. So I agree with your suggestion @uGoMobi. Mind tweaking the docs for this?

Jasper de Groot uGoMobi closed this in b7521e0 June 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.