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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

salcode
Copy link
Contributor

@salcode salcode commented Apr 23, 2015

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

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
Copy link
Contributor

@jtsternberg jtsternberg commented Apr 23, 2015

@jtsternberg
Copy link
Contributor

@jtsternberg jtsternberg commented Apr 23, 2015

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

@salcode
Copy link
Contributor Author

@salcode salcode commented Apr 23, 2015

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
Copy link
Contributor Author

@salcode salcode commented Apr 23, 2015

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
Copy link
Contributor

@jtsternberg jtsternberg commented Apr 23, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants