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

Remove dynamic social buttons #943

Merged
merged 5 commits into from
Oct 27, 2015
Merged

Conversation

rnagle
Copy link

@rnagle rnagle commented Oct 26, 2015

See #901.

This PR:

  • Removes the dynamic Twitter and Facebook buttons/links from the single post/page template near the byline and replaces them with simple links for the respective share service.
  • Removes references to largo_show_twitter_count since it's no longer relevant.
  • Updates tests related to largo_post_social_links.

@aschweigert
Copy link

does this affect the twitter/fb follow widgets? seems like it might. I'm ok with killing those but some sites are pretty wedded to them, the twitter one, particularly. we'll probably need some kind of plan to deal with that if we do nuke them.

@rnagle
Copy link
Author

rnagle commented Oct 27, 2015

@aschweigert Yeah, it definitely does affect the twitter and fb widgets. Didn't realize that at first. I added the required scripts back.

Kind of a bummer that we need to load these, but still think we're doing a good thing by not requiring the JS to power the dynamic share buttons.

<?php
// Are the widgets that contain facebook social buttons loaded (or are we on single/author?)
if( largo_facebook_widget::is_rendered() || largo_follow_widget::is_rendered() || is_single() || is_author() ) : ?>
if( largo_facebook_widget::is_rendered() || largo_follow_widget::is_rendered() || is_single() || is_author() ) : ?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the checks for is_single() and is_author() here if the scripts are no longer used as a result of the page being an author or single page?

@benlk
Copy link
Collaborator

benlk commented Oct 27, 2015

Tests pass and the buttons work. I have couple questions on the enqueueing made in comments on the diff.

It's a minor issue, but the Print button in the post-header social media buttons doesn't use the 'pointer' cursor used for the other links. This is understandable, because it doesn't have an <a>, but we should probably make it use the pointer cursor instead of a combination of the default arrow for the icon and the text-selection I cursor for the text of the button.

rnagle added a commit that referenced this pull request Oct 27, 2015
@rnagle rnagle merged commit e926a53 into develop Oct 27, 2015
@rnagle rnagle deleted the remove-dynamic-social-buttons branch November 3, 2015 15:43
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.

3 participants