Skip to content

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.

)
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.

)
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.

Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

can you please address the review comments?

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

3 participants