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

Implement dynamic meta tags #79

Merged
merged 21 commits into from
Apr 21, 2022
Merged

Implement dynamic meta tags #79

merged 21 commits into from
Apr 21, 2022

Conversation

erayalkis
Copy link
Collaborator

Description

This Pull Request implements dynamic meta tags for all plays using react-helmet.

Fixes #76

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Inspecting the section in the document inspector or copying and pasting the current url into a platform that supports embeds should let anyone test the new functionality.
On a local server though, you will have to use an external browser extension to verify that the meta tags are working as expected.
I didn't write any tests or make any changes to the documentation, as I didn't think it was necessary.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Notes:

I should mention that while I was testing the meta tags on localhost, I had to provide full paths for the meta tag (ex: "www.reactplay.io/static/exampleimage12345.png"), and I removed the domain from url before the final commit. If this causes the meta image to not work it should be super easy to fix, but I just thought I should note it down here.

I hope the new changes are good, let me know if I need to anything else !!

@vercel
Copy link

vercel bot commented Apr 20, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/atapas/react-play/2xJAa8j6T2CcshQQVp4DVwJHAJGZ
✅ Preview: https://react-play-git-fork-erayalkis-main-atapas.vercel.app

Copy link
Member

@atapas atapas left a comment

Choose a reason for hiding this comment

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

Do not commit package lock file., yarn lock enough.

src/common/playlists/PlayMeta.jsx Show resolved Hide resolved
@atapas atapas added work in progress The work is in progress review ready and removed work in progress The work is in progress labels Apr 21, 2022
@atapas
Copy link
Member

atapas commented Apr 21, 2022

Hello @erayalkis

We have upgraded to React v18 as part of this issue: #87

We have tested the existing plays. All good. Please pull this change to your branch and keep it up-to-date. You have to do sanity of things to make sure everything works for your changes in your branch.

Happy Coding!

@erayalkis
Copy link
Collaborator Author

I'm on it! Will ensure that everything works and push the updated version ASAP

@erayalkis
Copy link
Collaborator Author

All done, everything works as intended!

@atapas
Copy link
Member

atapas commented Apr 21, 2022

All done, everything works as intended!

Will start a final review soon. Thanks!

@atapas
Copy link
Member

atapas commented Apr 21, 2022

All done, everything works as intended!

Will start a final review soon. Thanks!

@erayalkis I have a question. How did you test the changes?

  • Home page
  • A play got a cover
  • A play without a cover(we have one called state)

If you could post the screenshots, it helps me reducing the test of it.

@erayalkis
Copy link
Collaborator Author

The new components introduce no UI or logic changes, so I had to test it manually using an external app, it's why the links look different on the screenshots. All of the pages look the same:
image
image
image
And when you post a link of a play on a platform that supports embeds (I personally used discord here), it looks like this:
image
If there's no cover image available, it just uses the default one instead.

Copy link
Member

@atapas atapas left a comment

Choose a reason for hiding this comment

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

@erayalkis Looks good.

@atapas atapas merged commit 0d497d3 into reactplay:main Apr 21, 2022
@erayalkis
Copy link
Collaborator Author

Alrighty! I'm so glad I could help!

@atapas
Copy link
Member

atapas commented Apr 21, 2022

@all-contributors please add @erayalkis for Code

@allcontributors
Copy link
Contributor

@atapas

I've put up a pull request to add @erayalkis! 🎉

@atapas
Copy link
Member

atapas commented Apr 21, 2022

Alrighty! I'm so glad I could help!

If you share your experience on Twitter, don't forget to tag ReactPlayIO :)

@erayalkis
Copy link
Collaborator Author

Will do! :)

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.

✨ [Feature request]: Change meta information based on the route
2 participants