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

Refactoring SchemaCollection to allow any kind of schema + internal code simplification #81

Merged

Conversation

QuentinGab
Copy link
Contributor

Hi,

The main purpose of this PR is to allow any kind of schema to be defined within SchemaCollection. Along the way, I also simplified some internal logic to simplify the code.

This PR does not introduce any breaking changes!

The new recommended and documented way to add schema is now as follows:

use RalphJSmit\Laravel\SEO\SchemaCollection;

public function getDynamicSEOData(): SEOData
{
    return new SEOData(
        // ...
        schema: SchemaCollection::make()->add(fn(SEOData $SEOData) => [
            '@context' => 'https://schema.org',
            '@type' => 'FAQPage',
            'mainEntity' => [
                '@type' => 'Question',
                'name' => 'Your question goes here',
                'acceptedAnswer' => [
                    '@type' => 'Answer',
                    'text' => 'Your answer goes here',
                ],
            ],
        ]),
    );
}

Additionally, this PR comes with 2 presets that replace addArticle and addBreadcrumbs.

Here's an example using withArticle:

use RalphJSmit\Laravel\SEO\SchemaCollection;
use RalphJSmit\Laravel\SEO\Support\SEOData;
use Illuminate\Support\Collection;

public function getDynamicSEOData(): SEOData
{
    return new SEOData(
        // ...
        title: "A boring title"
        schema: SchemaCollection::make()->withArticle(function(SEOData $SEOData, Collection $article){
            return $article->mergeRecursive([
                'alternativeHeadline' => "Not {$SEOData->title}", // will be "Not A boring title"
                'author' => [
                    [
                        '@type' => 'Person',
                        'name' => $this->moderator,
                    ]
                ]
            ]);
        }),
    );
}

I admit that this new way of modifying preconfigured schema is not entirely ideal. It could be greatly improved by defining new classes that extend Collection and support methods similar to the current Article.

However, considering the complexity of supporting every schema key in a fluent way, I'm not entirely convinced it's necessary at this time.

src/Schema/Schema.php Show resolved Hide resolved
src/Schema/Schema.php Show resolved Hide resolved
src/Support/Tag.php Show resolved Hide resolved
src/Support/Tag.php Show resolved Hide resolved
src/Tags/DescriptionTag.php Show resolved Hide resolved
Copy link
Owner

@ralphjsmit ralphjsmit 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 for the extensive work on it! Think it will be a great addition and super-useful. I went through the PR and left some comments with some things I noticed 👍

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Schema/ArticleSchema.php Outdated Show resolved Hide resolved
src/Schema/Schema.php Show resolved Hide resolved
src/Support/Tag.php Outdated Show resolved Hide resolved
src/Support/Tag.php Show resolved Hide resolved
src/Schema/Schema.php Show resolved Hide resolved
src/Support/Tag.php Show resolved Hide resolved
Copy link
Contributor Author

@QuentinGab QuentinGab left a comment

Choose a reason for hiding this comment

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

Thanks @ralphjsmit for your feedback, I answered all the comments I guess and I'm going the push new code regarding the deprecated methods

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Schema/ArticleSchema.php Outdated Show resolved Hide resolved
src/Schema/Schema.php Show resolved Hide resolved
src/Support/Tag.php Outdated Show resolved Hide resolved
src/Support/Tag.php Show resolved Hide resolved
@QuentinGab
Copy link
Contributor Author

I managed to refactor ArticleSchema and BreadcrumbListSchema to make them use SchemaTag (renamed from CustomSchema) without any breaking changes !!
I'm pretty happy with the result as the new Schema paradigm is now more fluent, extensible and simple to me

@ralphjsmit
Copy link
Owner

Thank you for the changes! Looking really good on first glance, I needed to get used to the the Schema extends Collection idea a bit but think it makes flexible indeed and better aligns with the new way of defining schema's. I will do a review and merge more in depth later around the end of the week . Thanks!

@QuentinGab
Copy link
Contributor Author

Hey @ralphjsmit did you get a chance to look at it ?

@ralphjsmit
Copy link
Owner

Sorry, not yet, likely next week.

@ralphjsmit ralphjsmit force-pushed the qgabriele/refactor-schema-collection branch from a4a3f51 to e73fe3c Compare June 15, 2024 12:18
@ralphjsmit ralphjsmit force-pushed the qgabriele/refactor-schema-collection branch from be8eae9 to e1ace45 Compare June 15, 2024 13:08
@ralphjsmit ralphjsmit merged commit 634d70f into ralphjsmit:main Jun 15, 2024
17 of 18 checks passed
@ralphjsmit
Copy link
Owner

Hi Quentin! Thanks for your patience, I came back to the PR today and reviewed it. In the end I decided to keep the fluent methods for article, breadcrumb list and faq page, because I think having these as options for fluent classes is valuable for developers as well, because it provides them with auto-completed fluent methods for the most common things to change, without having to remember the exact keys and format needed for these. I did refactor these 3 fluent classes to extend the CustomSchema paradigm, so we just have one paradigm for JSON schemas, and in the future if someone ever wants to create new fluent CustomSchema classes, they can just be added as easily. The with*() methods I did remove, so we ended up with 4 methods ->add(), ->addArticle(), ->addBreadcrumbs() and ->addFaqPage(), which I think is a nice combination. The FaqPage I also added back, since we already had it merged and I like the auto-completed syntax of it (why force people to manually construct an array if they can also use one neat method).

I just released it in 1.6.0, together with the alternate tag PR. Thanks for providing the PR to make it easier to add custom schemas! :)

@QuentinGab
Copy link
Contributor Author

@ralphjsmit Great! thank you very much 🙏

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