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

Update custom Navbar/Footer models & templatetags to work in multi-site #684

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jacklinke
Copy link

Description of change

Added a site FK field to the custom Navbar and Footer models in the pro template; added the new migration for these models; and updated the website templatetags in the pro template to work with the newly updated models.

Resolves #683 & #673

Documentation

Updated the docs the discuss how the snippets in the basic and pro templates work with multi-site installs in different ways.

Tests

Did not add tests, because there are no existing tests for the pro template website app (unless I missed something). Without an existing test suite for the website app in which to add my tests, I didn't want to make significant additions without asking the coderedcorp folks how/if I should approach this.

@vsalvino
Copy link
Contributor

Nice work, thanks! We can remove that fallback code as it is not necessary and should simplify it a bit.

Tests aren't necessary here, as long as you manually tested it. The pipeline will generate a pro project and run the full battery of tests using it.

@jacklinke
Copy link
Author

As requested.

Also, apologies for committing directly to main vice a new branch. I'm clearly a bit out of practice 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Please restore this file - it is needed in the project template.

@@ -221,9 +221,17 @@ class Meta:
],
use_json_field=True,
)
site = models.ForeignKey(
"wagtailcore.Site",
on_delete=models.CASCADE,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think SET_NULL would be preferable, in the event that you wanted to switch this navbar to a different site and deleted the site first. Similar to how the root page works.

@@ -249,9 +257,17 @@ class Meta:
blank=True,
use_json_field=True,
)
site = models.ForeignKey(
"wagtailcore.Site",
on_delete=models.CASCADE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto SET_NULL

@vsalvino
Copy link
Contributor

Looking good, left a few more comments. I think it will be ready to merge after that.

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.

Make the Custom Navbar/Footer in Pro Template work in Multi-Site Installs
2 participants