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

Twitter cards #375

Merged
merged 8 commits into from
Aug 25, 2019
Merged

Twitter cards #375

merged 8 commits into from
Aug 25, 2019

Conversation

alehel
Copy link
Collaborator

@alehel alehel commented Aug 25, 2019

Description

Currently Twitter cards don't contain a lot of information. This PR improves the look of these cards.

Twitter card with current code
image

Twitter card with this PR
image

You define the image used in the card by defining the socialImage tag in the md file for a page. If it hasn't been defined, we fall back to using your photo.jpg file. The twitter card description uses the existing description tag in the md file.

image

I've also updated the sample md files with the socialImage tag.

Related Issues

Related to #350

@alxshelepenok
Copy link
Owner

Wow, this is a very good solution.
It remains to fix a couple of linting errors

@codecov
Copy link

codecov bot commented Aug 25, 2019

Codecov Report

Merging #375 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   97.61%   97.68%   +0.06%     
==========================================
  Files          35       35              
  Lines         168      173       +5     
  Branches        9       10       +1     
==========================================
+ Hits          164      169       +5     
  Misses          4        4
Impacted Files Coverage Δ
src/templates/page-template.js 100% <100%> (ø) ⬆️
src/templates/post-template.js 100% <100%> (ø) ⬆️
src/components/Layout/Layout.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cdd6c9...849ef16. Read the comment docs.

@alehel
Copy link
Collaborator Author

alehel commented Aug 25, 2019

I forgot to update the tests. I've pushed a quick fix, but I must admit. I don't have a lot of experience writing such tests. Be nice if you could take a look @alxshelepenok

I'll fix the linting errors :).

@alehel
Copy link
Collaborator Author

alehel commented Aug 25, 2019

I'm a little confused by the linting errors. I'm currently getting the following for the Layout.js file,

16:17 error Expected a line break after this opening brace

If you look at Sidebar.js from the master branch it seems to be formatted the same, but without triggering an error. So I'm a little confused what I've done wrong.

@alxshelepenok
Copy link
Owner

@alehel if the number of props is more than 3

const Layout = ({
  children,
  title,
  description,
  socialImage
}: Props) => {

@alehel
Copy link
Collaborator Author

alehel commented Aug 25, 2019

@alxshelepenok ah, I see. Thanks, I'll fix it now.

I tried running 'yarn lint' before I pushed these changes. However I never got any of these errors. Just a bunch of errors about the linebreak being CRLF, which gets fixed by git anyway. Is there something I need to do to get this command to run correctly on Windows?

@alxshelepenok
Copy link
Owner

alxshelepenok commented Aug 25, 2019

@alehel strange, my preffered os is Fedora Linux, so I didn’t notice it.
To fix these errors, you can try adding linebreak-style: "unix" to eslint config.
diegohaz/arc#171

@alehel
Copy link
Collaborator Author

alehel commented Aug 25, 2019

@alxshelepenok thanks, I got linting working locally by adding "linebreak-style": ["error", "windows"], to the config file. I'll make sure not to push that change :p.

I'm now getting the expected linting errors. I guess the large number of CRLF errors were preventing the actual issues from surfacing in the terminal. Looks like all linting issues are fixed now.

Copy link
Owner

@alxshelepenok alxshelepenok left a comment

Choose a reason for hiding this comment

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

@alehel Thanks! It will not be superfluous to add an image for an open graph


return (
<div className={styles.layout}>
<Helmet>
Copy link
Owner

Choose a reason for hiding this comment

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

I also recommend adding an image for open graph
<meta property="og:image" content={metaImageUrl} />

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed 👍

@alxshelepenok
Copy link
Owner

Excellent thank you!

@alxshelepenok alxshelepenok merged commit 7488975 into alxshelepenok:master Aug 25, 2019
@alehel alehel deleted the twitter-cards branch August 25, 2019 12:56
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.

None yet

2 participants