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

feat: add Twitter meta tags to product page #530

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

edyespinal
Copy link

What problem is this solving?

This PR adds the possibility to add Twitter meta tags to a store's Product Page. The tags can be included by toggling the option in Admin > CMS > Store

The meta tags that are being added are the following:

<meta name=“twitter:card” content={ "photo" | "summary" | "summary_large_image" } />
<meta name=“twitter:site” content={ twitterUsername } />
<meta name=“twitter:title” content={ productName } />
<meta name=“twitter:description” content={ productDescription } />
<meta name=“twitter:image” content={ imageUrl } />
<meta name=“twitter:url” content={ productPageUrl } />

The settings added to the store's settingsSchema for this app can be found here and the logic to add the meta tags can be found here.

How to test it?

The Twitter Meta Tags and the Store apps are linked in this workspace.
To test it, make sure the includeTwitterMetaTags toggle is checked and add the Twitter Username and Twitter Card text fields are filled. Then visit any product page and look check for the Meta Tags that are being added.

Screenshots or example usage:

Screenshot 2021-07-28 at 16 21 28
Screenshot 2021-07-28 at 16 23 10

@edyespinal edyespinal requested a review from a team as a code owner July 28, 2021 14:32
@edyespinal edyespinal requested review from mullerjk, RodrigoTadeuF and pillarluz and removed request for a team July 28, 2021 14:32
@vtex-io-ci-cd
Copy link
Contributor

vtex-io-ci-cd bot commented Jul 28, 2021

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@vtex-io-docs-bot
Copy link

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

  • There's nothing new to document 🤔
  • I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

  • I just updated 🎉🎉

@victorhmp victorhmp requested review from icazevedo, igorbrasileiro and victorhmp and removed request for mullerjk, RodrigoTadeuF and pillarluz July 28, 2021 14:49
manifest.json Outdated Show resolved Hide resolved
Copy link
Contributor

@victorhmp victorhmp left a comment

Choose a reason for hiding this comment

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

Could you update the CHANGELOG file?
Also, please take a look into why the tests are failing. It's probably because there's no mock definition for vtex.twitter-meta-tags :)

CHANGELOG.md Outdated Show resolved Hide resolved
manifest.json Outdated Show resolved Hide resolved
manifest.json Outdated Show resolved Hide resolved
Copy link
Contributor

@igorbrasileiro igorbrasileiro left a comment

Choose a reason for hiding this comment

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

Thank you @edyespinal for the contribution 👏 !
Left some comments take a look at it.

CHANGELOG.md Outdated Show resolved Hide resolved
manifest.json Outdated Show resolved Hide resolved
react/__mocks__/vtex.twitter-meta-tags.js Show resolved Hide resolved
Remove unnecessary blank line.

Co-authored-by: Igor Brasileiro <brasileiro456@gmail.com>
manifest.json Outdated Show resolved Hide resolved
manifest.json Show resolved Hide resolved
"description": "admin/store.twitterUsername.description",
"type": "string"
},
"twitterCard": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This dropdown is not being rendered with title and description in the Site Editor for some reason... could you also look into that? It's just showing up as a dropdown with no context.

Screen Shot 2021-09-10 at 14 09 42

Copy link
Author

@edyespinal edyespinal Sep 16, 2021

Choose a reason for hiding this comment

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

Hi @victorhmp
I was doing a little digging to see what is causing the issue of the title and description not being displayed and the dropdown not having the correct style.
I noticed that we are using react-jsonschema-form to construct the store settings (for the apps' settings as well).
After reading the documentation, I don't see what we are doing wrong here. Maybe it's something when the VTEX Styleguide is being applied to the components.

After testing the same settings inside an apps settings form, this is what we get:

Screen.Recording.2021-09-16.at.14.49.45.mov

When testing the settings directly with react-jsonschema-form, this is what we get:

Screen.Recording.2021-09-16.at.14.44.18.mov

As you can see, in both cases the title and the description is displayed and the dropbox is styled correctly.

What should we do?

Copy link
Contributor

@victorhmp victorhmp Sep 17, 2021

Choose a reason for hiding this comment

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

@edyespinal this is awesome! Great investigation!
I have an idea! Could you try importing the Dropdown component from react/components/form/Dropdown and adding it to the widgets object created here? This might just do it!

Copy link
Author

Choose a reason for hiding this comment

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

Hi @victorhmp
I tried your approach but we are still getting the same results.

Copy link
Author

Choose a reason for hiding this comment

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

Hi team!
Is there anything else we need to do about this app?

CHANGELOG.md Outdated Show resolved Hide resolved
@davicostalf
Copy link

I think we need to solve this problem where the label of the field is not being shown before releasing the feature.
Also, are we supporting usernames written both with and without the @?

@edyespinal
Copy link
Author

Hi @davicostalf 👋🏼

I think we need to solve this problem where the label of the field is not being shown before releasing the feature.

Is there a workaround we can implement for this while the issue is fixed?

Also, are we supporting usernames written both with and without the @?

Yes, we took your comment into consideration and made the change. You can see it here. Also, the message was changed by removing the (Do not include @ sign) from all the languages.

@edyespinal
Copy link
Author

Hey team!
Any updates on this PR?

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.

5 participants