Adds AddThis social sharing and email buttons #279
Conversation
Codecov Report
@@ Coverage Diff @@
## master #279 +/- ##
==========================================
- Coverage 61.77% 61.15% -0.63%
==========================================
Files 16 17 +1
Lines 259 278 +19
Branches 54 54
==========================================
+ Hits 160 170 +10
- Misses 99 108 +9
Continue to review full report at Codecov.
|
app/App.tsx
Outdated
| @@ -10,6 +11,7 @@ import lazyLoad from './utils/LazyLoad'; | |||
| import './assets/css/main'; | |||
|
|
|||
| declare const System; | |||
| declare const window: any; | |||
There was a problem hiding this comment.
Extend the window interface here rather then stubbing out the entire object. Otherwise this could get dangerous down the road because you now have 0 type checking happening on window.
So, in globals.d.ts, do this...
interface Window {
addThis: TheInterfaceOfTheAddThisGlobalHere;
}This causes window to keep all its existing properties and also inherit whatever you set.
There was a problem hiding this comment.
Nice tip. Does the d.ts file automatically get recognized by the compiler as a definition file and applied to all sub-modules?
There was a problem hiding this comment.
As long as it exists somewhere in the project path (defined in tsconfig.json). In the case of globals.d.ts, it's at the root so it gets picked up. But it doesn't have to be at the root. It can be anywhere.
app/cards/Card.tsx
Outdated
| @@ -37,7 +43,7 @@ const config = { | |||
| }; | |||
|
|
|||
| @graphql(cardDataFromId, config) | |||
| export default class Card extends React.PureComponent<Props, {}> { | |||
| export default class Card extends React.Component<Props, {}> { | |||
There was a problem hiding this comment.
Just an FYI -- TypeScript now has the ability to set default generic parameters and the React type definitions now have them set appropriately.
So instead of React.Component<Props, {}> { you can simply write React.Component<Props> {.
If you have no props or state, you can now write just React.Component {
app/components/AddThis.tsx
Outdated
| } | ||
|
|
||
| componentDidUpdate() { | ||
| window.addEventListener('load', window.addthis.layers.refresh()); |
There was a problem hiding this comment.
Why are you adding new event listeners on every update? Didn't check, but this is almost certainly going to be a memory leak.
There was a problem hiding this comment.
You're right. Doesn't need it. This component doesn't get regular updates so wasn't a big deal, but you're right. Didn't need it. Really struggled with the lifecycle methods and where to put the refresh().
app/components/AddThis.tsx
Outdated
| } | ||
|
|
||
| render() { | ||
| const card_url = `https://www.aliemcards.com${this.props.url}`; |
There was a problem hiding this comment.
This is potentially dangerous here. XSS injection, etc.
Need to probably sanitize it or something to make sure its a good url without any query parameters or fluff.
There was a problem hiding this comment.
This is just the pathname passed down from React Router. No user input or url parameters used.
app/components/AddThis.tsx
Outdated
| } | ||
|
|
||
| componentWillUnmount() { | ||
| window.removeEventListener('load', window.addthis.layers.refresh()); |
There was a problem hiding this comment.
Forgot to mention here: This doesn't do anything.
window can't remove a function being called.
If you wan't to remove the load listener associated with refresh, you need to write it without the ().
Also, is it window.addthis or window.addThis???
There was a problem hiding this comment.
Good catch on the function invocation.
lowercase t.
|
It doesn't look like much in the end. But it took a lot of work to get there. Adding to the window event listener instead of react lifecycle hooks was the big fix. |
app/App.tsx
Outdated
| }; | ||
| } | ||
|
|
||
| handleScriptLoad() { | ||
| this.setState({ addthisLoaded: true }); |
There was a problem hiding this comment.
Doesn't matter really here, but try to use the setState variant that accepts a function. It's a touch safer so it's just a good habit to get into.
So in this case, it would be this:
this.setState(state => ({ ...state, addthisLoaded: true }));
app/components/AddThis.tsx
Outdated
|
|
||
| export default class Card extends React.Component<Props, {}> { | ||
| componentDidMount() { | ||
| window.addEventListener('load', window.addthis.layers.refresh()); |
There was a problem hiding this comment.
Remove the invocation here as well
|
@jvoros I just tried actually running this now for the first time and it doesn't work for me.... Does it work for you? |
| <div className="addthis_share"> | ||
| <div className="addthis_button"> | ||
| <i className="addthis_icon_facebook" /> | ||
| <div |
There was a problem hiding this comment.
Why is this div here? Can we put these data attributes on the enclosing div?
There was a problem hiding this comment.
The div with "addthis_share_button" has to be separate from the icon or the addthis script adds a link to the icon image without the appropriate data-attributes, even if the data-attributes are on the containing element.
| @@ -1,7 +1,12 @@ | |||
| jest.mock('react-apollo'); | |||
| const share = jest.fn(); | |||
| Object.defineProperty(window, 'addthis', { | |||
There was a problem hiding this comment.
This is fine here, but you might want to consider putting this into a setupFiles or setupTestFrameworkScriptFile so that it carries across all test environments.
|
@jvoros Status on this? |
|
Can we close this? |
No description provided.