350 open graph react #360

2018-02-23T20:32:10+01:00

bones7242 commented

2018-02-23 20:32:10 +01:00

(Migrated from github.com)

added server side rendering
added react-helmet to handle meta tags

+ added server side rendering + added `react-helmet` to handle meta tags

neb-b (Migrated from github.com) requested changes 2018-02-23 21:46:40 +01:00

neb-b (Migrated from github.com) left a comment

Looking good.

Main things to fix are moving the title/meta tag utilities into the SEO component to avoid unneeded duplication. And removing some of the extra console logs/old commented code.

My webpack comments are nice to have, but not super important for now

Looking good. Main things to fix are moving the title/meta tag utilities into the `SEO` component to avoid unneeded duplication. And removing some of the extra console logs/old commented code. My webpack comments are nice to have, but not super important for now

models/index.js

neb-b (Migrated from github.com) commented

2018-02-23 21:41:15 +01:00

I think you can delete this?

react/client.js

neb-b (Migrated from github.com) commented

2018-02-23 20:57:29 +01:00

Probably don't need these console.logs here.

Probably don't need these `console.log`s here.

react/components/OpenGraphTags/index.jsx

neb-b (Migrated from github.com) commented

2018-02-23 21:40:39 +01:00

I'm probably missing something here, but wherever this is rendered, don't you already have the asset? You could just directly pass it in (or just pass some identifier to easily find it in state). Then you wouldn't need the show reducer here, which I don't really think needs to exist (in the future).

Then you wouldn't have to worry about the show reducer being populated and could use these tags for other pages (not sure if you would ever need to do that though)

I'm probably missing something here, but wherever this is rendered, don't you already have the asset? You could just directly pass it in (or just pass some identifier to easily find it in state). Then you wouldn't need the `show` reducer here, which I don't really think needs to exist (in the future). Then you wouldn't have to worry about the `show` reducer being populated and could use these tags for other pages (not sure if you would ever need to do that though)

react/containers/LoginPage/view.jsx

					
				@ -15,24 +16,25 @@ class PublishPage extends React.Component {

				  render () {

neb-b (Migrated from github.com) commented

2018-02-23 21:25:05 +01:00

You can move all of these utility functions into the SEO component. Just pass in the minimum values needed and create the actual title/link/tags inside the component.

You can move all of these utility functions into the `SEO` component. Just pass in the minimum values needed and create the actual title/link/tags inside the component.

neb-b (Migrated from github.com) commented

2018-02-23 21:30:19 +01:00

Then you don't need to add the same three lines to every page component.
<SEO title="Login" link="login" />
or
<SEO title="Channel" link="channel" channel={channel} />

Then in the component you can check if channel or asset was passed in and build the meta tags accordingly.

if (!props.asset || !props.channel) {
    metaTags = createBasicMetaTags();
}

Then you don't need to add the same three lines to every page component. `<SEO title="Login" link="login" />` or `<SEO title="Channel" link="channel" channel={channel} />` Then in the component you can check if `channel` or `asset` was passed in and build the meta tags accordingly. ``` if (!props.asset || !props.channel) { metaTags = createBasicMetaTags(); } ```

webpack.config.js

neb-b (Migrated from github.com) commented

2018-02-23 21:17:39 +01:00

I would recommend creating separate webpack configs for dev/prod. Before we did our big restructure changes the apps webpack setup had three files:

A base config: contains the generic stuff like babel, entry/output points, css loader, etc.
A dev config which extends the base config: contains stuff like watch: true (not needed in prod) and source maps
A prod config which extends the base config: contains stuff like minification (that's the main thing)

webpack-merge is a great tool for this
https://github.com/survivejs/webpack-merge

I would recommend creating separate webpack configs for dev/prod. Before we did our big restructure changes the apps webpack setup had three files: A base config: contains the generic stuff like babel, entry/output points, css loader, etc. A dev config which extends the base config: contains stuff like `watch: true` (not needed in prod) and source maps A prod config which extends the base config: contains stuff like minification (that's the main thing) `webpack-merge` is a great tool for this https://github.com/survivejs/webpack-merge

bones7242 (Migrated from github.com) reviewed 2018-02-24 00:47:01 +01:00

react/components/OpenGraphTags/index.jsx

bones7242 (Migrated from github.com) commented

2018-02-24 00:47:01 +01:00

ah, yes. This component should actually be deleted. I made it before moving everything into the <SEO /> component.

ah, yes. This component should actually be deleted. I made it before moving everything into the `<SEO />` component.

neb-b (Migrated from github.com) approved these changes 2018-02-24 07:16:34 +01:00

Sign in to join this conversation.

No reviewers

neb-b

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/spee.ch#360

				`@ -15,24 +16,25 @@ class PublishPage extends React.Component {`
				`render () {`