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

Issue 117 - Update facebook javascript #121

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Issue 117 - Update facebook javascript #121

wants to merge 4 commits into from

Conversation

rodfersou
Copy link
Member

@rodfersou rodfersou commented Jul 25, 2017

WIP

Documentation: https://developers.facebook.com/docs/plugins/share-button

Todo list:

  • Fix the data-href to point to canonical_url
  • Remove options that don't exist anymore
  • Add new options
  • Question: How is the behavior with Do not track option activated.
  • Question: Is is a good idea to use mobile-iframe option?
    - The answer is Yes. It is a bad behavior for mobile to open a new tab. Of course they prefer if you use the native share option from mobile, but what if you didn't install the facebook app in your phone? you just can't share the page, and this option fix the behavior to don't jump to the new tab.
  • Fix tests
  • Update javascript and html
  • Add more tests

@rodfersou rodfersou force-pushed the issue_117 branch 2 times, most recently from 9512357 to 48d531a Compare July 25, 2017 11:52
var js, fjs = d.getElementsByTagName(s)[0];
if (d.getElementById(id)) return;
js = d.createElement(s); js.id = id;
js.src = "//connect.facebook.net/{0}/sdk.js#xfbml=1&version=v2.10";
Copy link
Member Author

Choose a reason for hiding this comment

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

This example from Facebook API is broken, it miss the &version part and then asks for the version at first run (I think it is related with locale change this error)

Copy link
Member

Choose a reason for hiding this comment

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

check the code produced by the widget, not the one in the example.

@@ -16,5 +16,5 @@
</tal:image>
<meta tal:condition="view/admins" property="fb:admins" tal:attributes="content view/admins" />
<meta tal:condition="view/app_id" property="fb:app_id" tal:attributes="content view/app_id" />
<script type="application/javascript" tal:content="view/fbjs"></script>
<script type="application/javascript" tal:content="structure view/fbjs"></script>
Copy link
Member Author

Choose a reason for hiding this comment

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

Use structure to produce the &


self.document.setLanguage('en')
view = document.restrictedTraverse(plugin_view)
html = view.metadata()
self.assertIn('connect.facebook.net/en_GB/all.js', html)
self.assertIn('connect.facebook.net/en_GB/sdk.js', html)
Copy link
Member Author

@rodfersou rodfersou Jul 25, 2017

Choose a reason for hiding this comment

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

Isn't it supposed to be en_US here? I think we have a Bug.

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea; you have to dig further on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The code that produces the language is not a simple one. I'll use this issue to check it out #122

@rodfersou rodfersou changed the title Issue 117 - Update plugins javascript Issue 117 - Update facebook javascript Jul 26, 2017
CHANGES.rst Outdated
@@ -6,6 +6,9 @@ There's a frood who really knows where his towel is.
2.10.1 (unreleased)
^^^^^^^^^^^^^^^^^^^

- Update plugins javascript (closes `#117 <https://github.com/collective/sc.social.like/issues/117>`_).
Copy link
Member

@hvelarde hvelarde Jul 26, 2017

Choose a reason for hiding this comment

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

this is not closing the issue; the issue serves only as a reference; the changelog entry must be something like: Facebook's widget code was updated… blah, blah, blah.

we'll take care of that later.

@rodfersou rodfersou force-pushed the issue_117 branch 2 times, most recently from ee60716 to 4634ba5 Compare August 1, 2017 13:05
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