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

Caching of menu and resolve urls of entries #79

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

Conversation

BobWez98
Copy link
Collaborator

@BobWez98 BobWez98 commented Oct 7, 2024

This Pull request contains a few things:

The parsing of urls will make sure if you select a category content, brand content and product content entry, the url will get resolved by either the normal statamic entry that's selected, of the runway resource that's linked to the collection. This way we only need one field in the navigation blueprint. This way we can also use the normal entry field in content components, and just use the helper to get the url. I also added some listeners to set the settings of the navigation right when it's created, and clear the cache of the navigation when it's updated.

Using the determineEntryUrl helper:

RapidezStatamic::determineEntryUrl($entry)

This will return a string for the href attribute.

Using the nav helper to fetch the navigation:

RapidezStatamic::nav('nav:main');

This will return an array of items with a resolved url.

composer.json Outdated Show resolved Hide resolved
resources/views/components/nav.blade.php Outdated Show resolved Hide resolved
src/Facades/StatamicNav.php Outdated Show resolved Hide resolved
src/Listeners/ClearNavTreeCache.php Outdated Show resolved Hide resolved
src/StatamicNav.php Outdated Show resolved Hide resolved
src/RapidezStatamicServiceProvider.php Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with just one config file with the new nav config added. Just to keep it simple and it's not that big (yet). config.rapidez.statamic.system feels odd.

config/rapidez/statamic/nav.php Outdated Show resolved Hide resolved
@@ -97,6 +103,9 @@ public function bootListeners() : self
Cache::forget('statamic-globals-' . Site::selected()->handle());
});

Event::listen(NavCreated::class, SetCollectionsForNav::class);
Copy link
Member

Choose a reason for hiding this comment

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

So when I change the config it's only applied on nav creation? When I change the config it should be applied to all navigations I think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, only when a new single navigation is created, this will set the allowed collections on that new single navigation. This are just defaults and when you need or feel like changing it you're free to do so.

src/RapidezStatamic.php Outdated Show resolved Hide resolved
src/RapidezStatamic.php Outdated Show resolved Hide resolved
src/RapidezStatamic.php Outdated Show resolved Hide resolved
Comment on lines 69 to 75
$suffix = str($linkedRunwayResourceKey)->contains('category')
? Rapidez::config('catalog/seo/category_url_suffix', '')
: (
str($linkedRunwayResourceKey)->contains('product')
? Rapidez::config('catalog/seo/product_url_suffix', '')
: ''
);
Copy link
Member

Choose a reason for hiding this comment

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

A match() could clean this up and make it better readable. But maybe we should move this to the models? So we're getting a route() method there to determine the url? Just like Runway handles frontend routing: https://github.com/statamic-rad-pack/runway/blob/7.x/src/Routing/Traits/RunwayRoutes.php

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've decided on a match here. Sadly this works by getting the entry from the navigation and then getting the runway resource. This runway resources returns as array and not as model, so we couldn't use any of the routing magic. On top of that the runway routing acts like actual routing, and we don't want that. We want the category routing from Magento.

public function determineEntryUrl(Entry|Page $entry, string $nav = 'global-link'): string
{
$cacheKey = $nav . '-' . config('rapidez.store');
$this->navCache[$nav] = Cache::get($cacheKey, []);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this causing a lot of cache lookups as it's looks like it's called for every entry?

@BobWez98 BobWez98 requested a review from royduin October 31, 2024 14:50
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.

4 participants