Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support profile, article, and book open graph type properties #22

Merged
merged 5 commits into from
Dec 22, 2018

Conversation

econdie
Copy link
Contributor

@econdie econdie commented Dec 19, 2018

following documentation/guidelines here http://ogp.me/ this should allow the use of these different properties available for a few different open graph types.

let me know what you think?

example config of a profile page passed to NextSeo component:
{title: 'my page title', desciption: 'my page description', noindex: false, openGraph: { url: 'http://mypageurl.com', type: 'profile', profile: { first_name: 'John', last_name: 'Smith', username: 'jsmith123', gender: 'male' }, images: [ { url: 'link to profile image..' } ], title: 'open graph title', description: 'open graph description' } }

Copy link
Owner

@garmeeh garmeeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @econdie thanks very much for the contribution. I have tested what you have there so far and it's looking great. I have left some suggested changes on the pull request. Also I am going to add script for running prettier on the code, would be great if you could run it on your code.

I would be willing to accept the PR with just the suggested changes I have made but if you were up for it, the following would be ideal:

  • Add 3 new tests cases to buildTags.spec.js to test for the new Open Graph scenarios
  • Add 3 new pages to this directory that implements a full example of each new Open Graph type
  • Add 3 new test cases here to test the integration

I can work with you on them, or give more details if needed.

It will need some documentation but I can add that when preparing the release notes.

Note:
Multiple authors and tags is only supported from Next@7.0.2-canary.35 onwards. This is due to vercel/next.js#5800 making it possible.

src/meta/buildTags.jsx Outdated Show resolved Hide resolved
src/meta/buildTags.jsx Outdated Show resolved Hide resolved
src/meta/buildTags.jsx Outdated Show resolved Hide resolved
src/meta/buildTags.jsx Outdated Show resolved Hide resolved
src/meta/buildTags.jsx Outdated Show resolved Hide resolved
@garmeeh
Copy link
Owner

garmeeh commented Dec 21, 2018

@econdie I have just merged #24 which fixes the formatting up. If making changes could you please merge master into your branch first. If you then run npm install and then run npx prettier --write src/meta/buildTags.jsx.

@econdie
Copy link
Contributor Author

econdie commented Dec 22, 2018

@garmeeh the seo.spec.js is failing on the og images. on my new pages I am setting the images array with two objects. (the default config sets 4 objects). when the pages are rendered it is taking the base config and replacing two of the images with the new ones i specified. I'd expect none of the images to be included from the base config, and to only include images from my overridden configuration.

the test is failing because i am expecting that image length to be 2, but theres actually 4 images on the page from my explanation above.

is that a bug or expected behaviour?

@garmeeh
Copy link
Owner

garmeeh commented Dec 22, 2018

@econdie so this is expected behaviour if you set 4 images as the default SEO. Thinking about it now, that might not be the best recommendation as you would have to override 4 on each page.

For now, can you pass in an extra 2 images for each of your new pages?

I will swing back around on master and update the documentation and tests to reflect a better recommendation of only setting one default image. Good spot, I had overlooked this.

@econdie
Copy link
Contributor Author

econdie commented Dec 22, 2018

ok thanks for the explanation, i would agree on recommending setting 1 default image.

By the way this can lead to scenarios where if you want to specify image properties for a default image (i.e. specify width and height of the image) in your default config, but then on some page you want to override image to a new one specific for that page content but dont necessarily know the width/height of this image (so you leave it out of that page config), its going to pull that width/height from the default image which doesnt seem right. If thats the intended implementation no worries, just something that wasn't intuitive to me.

thanks for looking at this and let me know if i need more changes here

Copy link
Owner

@garmeeh garmeeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks very much for making the suggested changes. I will update the documentation and hope to get a release out tomorrow evening.

Also thanks for highlighting an issue that I had completely missed because of my use cases around defaults. I will update the recommendations around this and highlight the caveats of leveraging defaults.

@garmeeh garmeeh merged commit a52e6c6 into garmeeh:master Dec 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants