carrot / share-button

:warning: :warning: Currently Unmaintained :warning: :warning: - fast, beautiful, and painless social shares:
http://sharebutton.co/
Other
2.95k stars 381 forks source link

Fixed FB share double render error. #287

Closed joelmats closed 8 years ago

joelmats commented 8 years ago

simple fix for this issue.

hhsnopek commented 8 years ago

note: fixes #285

hhsnopek commented 8 years ago

hey @joelmats it seems that this change breaks the tests, could you look into why?

joelmats commented 8 years ago

Sure will do. Installing xcode atm which is taking a bit.

hhsnopek commented 8 years ago

I'd like to get this in the next release tomorrow morning :smile: - lemme know if you need any help adding the second test!

joelmats commented 8 years ago

@hhsnopek Ok just got back to it. I added a method in the widget to simulate an sdk load. Also one other thing, I had to change the driver url in the facebook test as it was using something like localhost:8000 for some reason. But the facebook test from master kept timing out on me so I switched it to read the html file directly.

joelmats commented 8 years ago

@hhsnopek Let me know if you want me to make any changes. I am also eager to get this in :)

hhsnopek commented 8 years ago

Just making sure i'm on the right track with this, now (only if you're using the sdk), facebook sharing will load on the current page. If you're not using the sdk it will create a pop out?

joelmats commented 8 years ago

Correct :)

On Wed, Feb 17, 2016 at 12:31 PM, Henry Snopek notifications@github.com wrote:

Just making sure i'm on the right track with this, now (only if you're using the sdk), facebook sharing will load on the current page. If you're not using the sdk it will create a pop out?

— Reply to this email directly or view it on GitHub https://github.com/carrot/share-button/pull/287#issuecomment-185390531.

Joel Rikiya Matsumoto

Mobile #: 650-515-7115

hhsnopek commented 8 years ago

Would you you be able to get the functionality of it to pop out even when the sdk is in use to match the other networks?

joelmats commented 8 years ago

Hmm not sure what you mean. If the SDK is loaded, facebook loads their own version of the popup. If a user does not want to use this version, they can just set the config to not use the sdk i think and it will fallback to the php popup.

On Wed, Feb 17, 2016 at 12:34 PM, Henry Snopek notifications@github.com wrote:

Would you you be able to get the functionality of it to pop out even when the sdk is in use to match the other networks?

— Reply to this email directly or view it on GitHub https://github.com/carrot/share-button/pull/287#issuecomment-185391506.

Joel Rikiya Matsumoto

Mobile #: 650-515-7115

hhsnopek commented 8 years ago

oh alright that's perfect then - if you could just separate out the sdk test from the original facebook test that would be great (then we'll have two different configs), still want to test against both :smile:

joelmats commented 8 years ago

Got it :)

On Wed, Feb 17, 2016 at 12:44 PM, Henry Snopek notifications@github.com wrote:

oh alright that's perfect then - if you could just separate out the sdk test from the original facebook test that would be great (then we'll have two different configs), still want to test against both [image: :smile:]

— Reply to this email directly or view it on GitHub https://github.com/carrot/share-button/pull/287#issuecomment-185395543.

Joel Rikiya Matsumoto

Mobile #: 650-515-7115

joelmats commented 8 years ago

@hhsnopek I added 2 tests since there are actually 3 cases. 1. User specifies enabled SDK, SDK loads properly, no url needed. 2. User specifies enabled SDK, SDK does not load (for some reason), url falls back to php popup. 3. User disables SDK load, SDK does not load, and url is set to the php popup. Let me know if this makes sense :)

hhsnopek commented 8 years ago

Beautiful - once travis is complete if you could squash the commits into one nice commit, we'll go ahead and merge this bad boy in :smiley:

joelmats commented 8 years ago

Rebased and squashed to 1 commit. Cheers!

hhsnopek commented 8 years ago

thanks again! :smile:

joelmats commented 8 years ago

Np. Thanks for the help and awesome plugin!

On Wed, Feb 17, 2016 at 1:15 PM, Henry Snopek notifications@github.com wrote:

thanks again! [image: :smile:]

— Reply to this email directly or view it on GitHub https://github.com/carrot/share-button/pull/287#issuecomment-185407737.

Joel Rikiya Matsumoto

Mobile #: 650-515-7115

rcamposp commented 8 years ago

Please make a new release with these changes.

Took me a couple of hours to fix it.