Add options shared templates

[raphcollective]

This is related to the issue #2

Hi everyone!

We needed this functionality for an upcoming project and decided to give it a go, this is still in a very draft state as I had to do some refactoring and it probably breaks the tests and documentation. I followed the few instructions from the original issue and came up with a solution that should both answers your needs and ours. I'm not sure what was the extend of your original idea but I will explain below what I did. Also I tried my best to follow your code style but didn't have enough time to make everything really clean, so I expect you to have some feedbacks on that as well.

Registering a new option template

For that part I followed the idea of @voidgraphics in the original issue and implemented it in the TemplatesRepository with a registerOptionsTemplate function called at the same time than the registerRouteTemplates.

The Options classes are just extending templates nothing changes here. They are registered in the novapage config file like this:

'options' => [
        'App\Nova\Options\FooterBlocks' => '*',
        'App\Nova\Options\ContactInfo' => ['home', 'contact']
]

To be able to keep the same class for the Options I had to do a small refactoring and set the template key attribute manually instead of computing it from the name and type variable. This is because we can't use the routes as the name as an option shares its values with all the routes it's bound to. The key is still used to determine whether the options values should be loaded for the current route. On options though the name attribute will be the class name.

This is the typical path that options will use to be stored: resources/lang/option/ClassName.json

I didn't make a command to generate the files in the correct path and add them to the options config yet but I could look into it after I have your initial feedbacks.

Editing the options values

This is where things get a bit more complex, I didn't really know what you wanted to do on that aspect so I had to make some decisions. The first one I did is to have the options as a separate resource, I think it makes sense as some fields become irrelevant like the title or the creation date. Also it makes everything less confusing, other similar tools like ACF for WP use that pattern too.

I created a OptionResource with the controllers that go along with it, and added a new sidebar menu entry called "Options" (by default it uses a options_label config variable like the other resource). This also means I had to refactor the existing Resource and controllers to be PageResource instead. I removed the base attributes panel and replaced it by a Heading field with the class name formatted properly.

I also refactored the QueriesPages file to have queries per template type. This is one of the things where I think there's probably a cleaner way of doing it.

Since we're using the template class I didn't have to make any modifications to the sources, everything is stored properly like the pages.

Accessing the options values

I decided not to add too much complexity to the manager by adding conditions in the magic functions, instead I added a new method getOption. It uses a new property array options which contains all the loaded options for the current route.

This array is filled up in the loadForRoute function and uses a filtered template array from the TemplatesRepository.

Blade directive

Lastly since we can't use the same method to get the options attributes I added an @option directive which works like this:

@option('FooterBlocks.copyright')

And that's it, sorry for the very long explanation but it should give you all the keys to understand the approach. Speaking of which I'm totally open to discussion if you have any feedback on any point, I hope that it matches your initial ambition.

Let me know if you need more details on anything.

[voidgraphics]

Wow! Thank you very much for putting time into this and sharing your code back with us. We've been very busy the past couple weeks, which explains why this package's development has slowed down, but we'll do our best to review this ASAP, as it is a pretty major feature.

[raphcollective]

Heya,

Just wanted to check on the status of this, also had a question regarding the options binding.

I've encountered an issue with the wildcard option as it takes routes from the nova namespace as well. How should we deal with this? Should we make an option to provide a list of namespace instead of the wildcard? Or do you want to keep it as it is?

Let me know and I will update the PR accordingly.

[voidgraphics]

Hey @raphcollective, this is next on our open-source to-do list. We had a pretty critical bug to fix on our flexible field yesterday and now we can focus on this when we get a minute. Thanks for your patience.

Seeing as this is a pretty large PR, I am going to wait until we have reviewed your code before answering your question about the wildcard problem. I think I should be able to do that today.

I do have a question however, while I understand loading Options for all routes using the wildcard is not great if it loads them on Nova routes too, how did you notice this problem? Did it break Nova in any way?

[voidgraphics]

@raphcollective, after pulling your PR, it seems that I can not update values on pages anymore. Can you verify that this is working on your end?

What I did was:

• Create a brand new Laravel project
• Installed Nova & your PR,
• Created a template using the following command: php artisan make:template Home, gave it a simple text field
• Registered the template on a route
• Tried to edit the template in Nova

When clicking the "Update nova-page" button, nothing happens. Nothing gets logged, no errors shown. The data does not get written to disk.

As this was not working, I did not check any of the Options stuff yet.

[raphcollective]

Hey @voidgraphics this branch doesn't include the latest fixes for Nova 2.0.1, could you try again on 2.0.0 and see if it's any better please?

[voidgraphics]

That was it, thanks.

[voidgraphics]

Okay, happy to report everything seems to be working fine. You've done some good work @raphcollective. I haven't looked into the details yet (this was all the time I had for this today) but I can tell some refactoring is indeed needed.

For example, the controllers for Pages and for Options have a lot of duplicated code, which could be refactored into a parent class.

Also, it would be nice to have these controllers live in their own folder based on their type, for example:

Http/Controllers/Page/UpdateController.php

instead of

Http/Controllers/PageResourceUpdateController.php

We will have more feedback for you once @Nyratas and I have some time to sit together and have a proper look at it.

[raphcollective]

Thanks for the review, keep me posted!

If everything is working correctly I'll wait for the full feedback to make any changes to the code then.

[raphcollective]

Hey @voidgraphics sorry just noticed this question today:

I do have a question however, while I understand loading Options for all routes using the wildcard is not great if it loads them on Nova routes too, how did you notice this problem? Did it break Nova in any way?

The issue is that if $throwOnMissing is set to true and the wildcard is used then you'll never be able to set it in nova because it will throw on nova routes.

[voidgraphics]

Hi @raphcollective, just to let you know, we haven't forgot this! We have some time planned in early June to work on our open source projects and this is first on the list. As I previously stated, some refactoring is needed and we will go into more detail then.

Again, your contribution is very much appreciated. 👍

[raphcollective]

Hey @voidgraphics! No worries, we know how hard it is to allocate time to that kind of project. I'll also take this occasion to thank you and @Nyratas for the support you've given us until now, we've been using nova-page and flexible-content on multiple productions and it's been a lifesaver.

[raphcollective]

Hey @voidgraphics and @Nyratas, do you guys have an idea of when you'd be able to start a proper code review? Like I said in my previous message I totally understand that these things take time, we used this fork on 2 production websites already but I'd love to consolidate some parts.

Let me know if I can be of any help!

EDIT: I just pushed some refactoring on the controllers and queries traits to remove duplicated code like discussed above

[toonvandenbos]

Hi @raphcollective, I'm working on your pull request today. Hope to get it up, running and merged this day.

[toonvandenbos]

The feature is ready to be merged.

Some important changes have however been added. Instead of defining the option pages in the config/novapage.php file, Option Template should now be registered in a ServiceProvider. This makes more sense since they're part of the app's logic and not the package's configuration.

I also updated the README and documentation files which will give more details on Option Templates definition and usage.

Hope you'll enjoy the new feature!

Thanks again to @raphcollective and @voidgraphics for the original pull request and insights.

[raphcollective]

Thank you so much @Nyratas for taking the time to finish this (and @voidgraphics for the initial review and idea)

I agree with you on the registration of the templates, it makes much more sense. Using this branch on our projects this was one of the pain points of the implementation, it was also a bit confusing for the users not familiar with the project.

Looking forward to see these changes merged and I will keep an eye out for any issues on it and give a hand if needed.