search

WebDevStudios / Hash-Link-Scroll-Offset

Offset the scroll position of anchored links. Handy if you have a sticky header that covers linked material.
14 stars 5 forks source link

Auto increase offset by 32px when admin bar present #5

Closed salcode closed 9 years ago

salcode commented 9 years ago

Modify the JavaScript to detect the presence of the WP Admin Bar and when present increase the offset value by 32px.

Add description to "Hash Link Scroll Offset" setting page to clarify the value is auto-incremented when the Admin Bar is present.

Fixes #4

jtsternberg commented 9 years ago

Merged, thank you @salcode! https://github.com/WebDevStudios/Hash-Link-Scroll-Offset/commits/master

jtsternberg commented 9 years ago

BTW, I ended up using is_admin_bar_showing() instead of checking for $('#wpadminbar').

salcode commented 9 years ago

Originally my solution used is_admin_bar_showing()

I liked the check for $('#wpadminbar') better because then the logic for changing the offset is completely client side.

My thought was it lent itself better to dealing with cached output. Reflecting on this, I'm not sure there is a situation where the localized hlso_data would be cached but the presence or absence of the $('#wpadminbar') would not. Based on this, setting the admin bar state in hlso_data server side should work just as well.

Are there any cases where the #wpadminbar would be present but hidden? (in which case, you'd need to check for the presence and check the value of the CSS display property). I don't think this is a situation that arises.

salcode commented 9 years ago

Mark Jaquith's Cache Buddy came to mind as an edge case but based on the paragraph copied below, I think it is pretty safe to say the server side solution is fine. Sorry to play this whole thought process out in this issue.

I think the current solution is fine. Thanks.

What about the toolbar?

Well, by default, Subscriber and Contributor users won’t see it. But it honestly isn’t very useful to them anyway. But Authors, Editors and Administrators (who should be a very small percentage of viewers) will still get dynamic page views like they do now, and they’ll see the toolbar.

jtsternberg commented 9 years ago

Ugh, I like your reasoning... I impulsively re-worked it thinking it would check $('#wpadminbar') with every click (that wasn't the case). the teeny tiny perf. improvement is def. not worth it if we can compensate for aggressively cached pages.