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

fix(og): fixed open graph field names #97

Merged
merged 2 commits into from
Jan 30, 2025
Merged

fix(og): fixed open graph field names #97

merged 2 commits into from
Jan 30, 2025

Conversation

Kvintus
Copy link
Contributor

@Kvintus Kvintus commented Jan 4, 2025

Summary

The : character is not allowed in field names when the GraphQL plugin generates type definitions. This restriction causes the app build to fail when both the GraphQL plugin and the SEO plugin are used, specifically when an SEO component is added to a content type.

Changes

  • Updated the naming of Open Graph fields in the SEO plugin to remove the : character, resolving the build failure.
  • Ensured compatibility between the GraphQL and SEO plugins, including fixing the issue where the preview functionality in the admin panel stopped working.

Screenshots

  • Attached a screenshot illustrating the build error:

Screenshot 2025-01-04 at 18 06 14

  • Working preview with the new attribute names:

Screenshot 2025-01-04 at 20 02 42

Related Issues

This PR addresses the issues raised in:

Both issues suggest renaming attributes as a potential solution but also note that the preview feature in the admin panel stops working. This PR resolves both the build failure and the preview functionality issue.

Testing

  • Verified that builds now complete successfully with both plugins enabled.
  • Confirmed that the preview functionality in the admin panel works as expected.

`:` Isn't allowed character for a field when the grapqhl plugin tries to generate definitions for it. 

This makes the build of the app fail if one has both Graphql Plugin and Seo plugin and adds an seo component to some content-type. 

Changing the names of the open-graphql components to not include the `:` fixes this bug.
@Kvintus Kvintus mentioned this pull request Jan 4, 2025
@Kvintus
Copy link
Contributor Author

Kvintus commented Jan 8, 2025

@jhoward1994 Hi sorry to directly mention you but, I'd be nice if we could get this merged as soon as possible and you are the person I remember being most active in these types of PRs.

Could you please look at it or assign it to someone ?

@zaosoula
Copy link

+1

4 similar comments
@mj0ln1r
Copy link

mj0ln1r commented Jan 15, 2025

+1

@bogdewi4
Copy link

+1

@kcelsi
Copy link

kcelsi commented Jan 22, 2025

+1

@sergeynagorny
Copy link

+1

@jhoward1994 jhoward1994 requested a review from Mcastres January 23, 2025 12:54
Copy link
Contributor

@jhoward1994 jhoward1994 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR and for all your patience with this 🙏🏻. To me this PR looks good & is working in conjunction with the graphQL plugin.

@Mcastres would you be able to take a look at this too please?

@GamesProSeif
Copy link

This PR needs to be merged ASAP as it can produce this serious issue strapi/strapi#18664

Copy link
Collaborator

@Mcastres Mcastres left a comment

Choose a reason for hiding this comment

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

LGTM

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.

9 participants