Closed sdotson closed 7 years ago
@sdotson It seems like a test is broken too
Thanks everybody for reviewing. I certainly learned a few things.
Oh Lord, do I feel silly. There's no need for the facebook sdk for a social share link. There's a simpler way with https://www.facebook.com/sharer/sharer.php?u=
.
I just now reverted most changes, implemented a much simpler url approach, and made sure both twitter and facebook links opened in a new window.
What do you all think about merging what we have here and creating a new issue to make this a server-rendered universal react app so we can do the dynamic meta tags for facebook/twitter share? Not sure how much work that would entail but I would find out after some googlin'/stackoverflowin'.
LOL, funny you mention that...initially it was server-side-rendered, but a lot of us are new to server-side rendering and got stuck on tasks that we would have been able to complete if not for the server-side rendering learning curve. So we went back to client-side rendering.
Anyhow, my vote is not to spend too much more time on this, we can always refine it later.
@sdotson If you don't mind posting a screenshot of how the popups look, as long as they look OK and the unit tests are passing, then I say we merge it and refine as needed in future iterations?
@dorono sure thing.
Arash mentioned open graph: https://developers.facebook.com/docs/opengraph/getting-started
. Looks like you started with that approach. This is probably the way to go to include image, title and date in the shares.
This is tackling #73.
It is currently using a dummy app id that I created, but will be updated with the official one once that is created. I'm not entirely sure about my approach, so I'm absolutely open and humble to feedback!
Specific things I wasn't sure about:
One final note is that the facebook share dialogue won't work with localhost links.