-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
refactor social media widgets #1115
Conversation
nm, fb widget code was just way out of date |
almost there, would still like to:
|
this will also eventually address #219 |
@jackbrighton you said as far as you could tell the twitter widget just needs the widget ID, right? none of the other parameters for the various widget types make a difference anymore? |
Yeah but that was a month ago or so while handling a related ticket and doing lot of other stuff. I'd like to quickly verify before we strip out code. |
ok. no worries, just wanted to further test that hypothesis and make any necessary changes as we address this PR. found out, for example that the version of the fb like box we were using was no longer supported (hadn't been for over a year :( |
@jackbrighton fyi re: the twitter widget, it's true that the text/url are not strictly necessary, but they are used as a fallback if the widget js doesn't load so it's probably worth keeping them |
Thanks, that's good to know! Sent from my iPhone
|
Just a note that Twitter search widgets appear to be dead now. Probably a minor issue, but that's a thing. |
This is good to go. We may at some point want to make it easier to add additional social networks to the regular theme options page but punting on that now. For a sample implementation of how to add a new social network to show in the follow widget and the row of icons in the header/footer, see: https://bitbucket.org/projectlargo/theme-gijn/src/cd95b21eb2bfc7ab2d338eaff1d5a9d1ff0149f4/inc/options.php?at=master&fileviewer=file-view-default Note that you also need some CSS for whichever button you're adding if the icon you want is not included in our current fontello set. |
still to do: