Skip to content
This repository has been archived by the owner on Jan 10, 2019. It is now read-only.

Localize loading of facebook javascript #3

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

Conversation

mlusetti
Copy link

Hi,
I've done some experiment and this is going to format the code more in sync with the Tapestry repository and more important to add Localization support loading the correct javascript SDK from Facebook depending on the language from the browser.

Oh... I whish this to depend on the current Tapestry trunk... I probably could have done better branching...

Please have a look and tell me what you think.

BTW Thanks for sharing this code has speed me up quite a lot

@trsvax
Copy link
Owner

trsvax commented Mar 25, 2011

I'll take a look. I'm working on the code right now. Since you might run into the same problem I have here's what I'm working on

Apparently linkSource returns a URL with with jsessionid. This causes a problem because now facebook thinks every url is different. Currently I'm just experimenting but I suspect I'll add this into the library somehow

    String url = linkSource.createPageRenderLinkWithContext(Index.class, painting.getTitle().replaceAll(" ", "_")).toAbsoluteURI();
    if ( url.indexOf(";") > 0 ) {
        url = url.substring(0, url.indexOf(";"));
    }

On Mar 25, 2011, at 11:45 AM, mlusetti wrote:

Hi,
I've done some experiment and this is going to format the code more in sync with the Tapestry repository and more important to add Localization support loading the correct javascript SDK from Facebook depending on the language from the browser.

Please have a look and tell me what you think.

BTW Thanks for sharing this code has speed me up quite a lot

Reply to this email directly or view it on GitHub:
#3

@mlusetti
Copy link
Author

Where do you think it will go? ... Like component? Everywhere you need a callback url? ... That seem feasible.

Have you had time to look into the code?

@mlusetti
Copy link
Author

I've updated the commit range since I've gone further and integrated more work, from me and from the fork from ascandroli..

Cheers

@trsvax
Copy link
Owner

trsvax commented Mar 28, 2011

I saw a comment in your code about requiring 5.3. What change did you make that requires 5.3?

On Mar 28, 2011, at 4:42 AM, mlusetti wrote:

I've updated the commit range since I've gone further and integrated more work, from me and from the fork from ascandroli..

Cheers

Reply to this email directly or view it on GitHub:
#3 (comment)

@mlusetti
Copy link
Author

Nothing, I simply had in mind to work with the "current trunk" since I'm used to it and it's tapestry-facebook is in alpha stage... I'f you care to have it on 5.2.4 lets move back to 5.2.4

@trsvax
Copy link
Owner

trsvax commented Mar 28, 2011

OK, I looked around and I did not see anything I thought depended on 5.3. I'd say lets leave is at 5.2.4 for now. Then I'll pick up your changes. I'm new to this git thing what commands do I need to run to pick up your changes.

Also, I think I'm going to setup a repository this week so people don't have to build the project.

Thanks for your interest. Hopefully the library save people from having to figure out the FaceBook api.

On Mar 28, 2011, at 7:22 AM, mlusetti wrote:

Nothing, I simply had in mind to work with the "current trunk" since I'm used to it and it's tapestry-facebook is in alpha stage... I'f you care to have it on 5.2.4 lets move back to 5.2.4

Reply to this email directly or view it on GitHub:
#3 (comment)

@mlusetti
Copy link
Author

Fine... I've done that, let me know If you get trouble.

Please note the the event callback features (FB.event.subscribe) needs some more love...

@mlusetti
Copy link
Author

BTW Tapestry 5.2.5 has been just voted as a successful release so I guess It could be bumped as soon as you like

@trsvax
Copy link
Owner

trsvax commented Mar 28, 2011

I would agree with that. I'd also like to support Zone updates on events. I need that for what I'm working on so I expect I'll have that soon

On Mar 28, 2011, at 8:01 AM, mlusetti wrote:

Fine... I've done that, let me know If you get trouble.

Please note the the event callback features (FB.event.subscribe) needs some more love...

Reply to this email directly or view it on GitHub:
#3 (comment)

@mlusetti
Copy link
Author

Yeah, that's would need a callback from FB.event.subscribe and I'm working on it but it gets screwed due to the async nature of the FB.init and the deferring of the javascript that Tapestry do...

Maybe I'll do a branch to work parallel with you... let's see it as soon as you pull changes in... and ... thanks for the effort in this library...

…s FB init call.

Now we do sync issues since Tapestry already defer the call after the page has loaded... nice
@mlusetti
Copy link
Author

Now the callback works as expected an we can start to use for Zone integration.

@trsvax
Copy link
Owner

trsvax commented Mar 29, 2011

I merged the changes and I noticed you switched from the async interface to the sync one. I'm not opposed to that but can you give me a quick reason. My impression was the async interface was perhaps faster but there is not a lot of documentation about that. It does seem trickier to interface with because I'm guessing fbAsyncInit could be called before the Tapestry libs are loaded.

If we head this way I'll update the README and I guess we'd need to change AsyncInit to FBInit

On Mar 28, 2011, at 9:58 AM, mlusetti wrote:

Now the callback works as expected an we can start to use for Zone integration.

Reply to this email directly or view it on GitHub:
#3 (comment)

@mlusetti
Copy link
Author

As, somehow, I said in my commit log this way we could easily integrate with the initialization of the tapestry.js namespace using the "tapestry way", seems more natural to me.

For the "performance reasons" I guess we could live with this as the first integration step then I will look at how to enahnche it but don't forget that the way tapestry javascript initialization call our FB init is deferred till the the page DOM is completely loaded so you're sure to call js function on Elements that actually exists, this is a big win.

For the renaming I would simply call it Init if you have to rename it somehow...

@trsvax
Copy link
Owner

trsvax commented Mar 29, 2011

OK lets head down the sync path and if needed we can revisit the async interface. I think it should be possible to make it work either way without changing the interface too much.

I'll clean up a few things and check in the merged version. My schedule is a bit busy for the next few days so it may be Friday before I get to it.

On Mar 29, 2011, at 7:21 AM, mlusetti wrote:

As, somehow, I said in my commit log this way we could easily integrate with the initialization of the tapestry.js namespace using the "tapestry way", seems more natural to me.

For the "performance reasons" I guess we could live with this as the first integration step then I will look at how to enahnche it but don't forget that the way tapestry javascript initialization call our FB init is deferred till the the page DOM is completely loaded so you're sure to call js function on Elements that actually exists, this is a big win.

For the renaming I would simply call it Init if you have to rename it somehow...

Reply to this email directly or view it on GitHub:
#3 (comment)

@ascandroli
Copy link

I love what Massimo did with the code.
I've done a merge in my own repository, including the 5.2.5 upgrade.
You will find a lot of conflicts due to text formatting, I highly recommend you use the "theirs" option when you pull/merge.

@mlusetti
Copy link
Author

This library needs some work on the test source base... maybe with Spock...

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

Successfully merging this pull request may close these issues.

3 participants