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

Implemented wagtail cookiecutter #250

Merged
merged 48 commits into from
Nov 30, 2022
Merged

Conversation

antidipyramid
Copy link
Contributor

Overview

My implementation of a Wagtail Cookiecutter.

Handles #249

Notes

Though I haven't found any stray tags and the Docker containers runs locally, this involved a lot of search and replace text operations. Please be on the lookout for errors caused by incorrect or missing Cookiecutter template tags or hardcoded app/module names.

Testing Instructions

After cloning the repo, follow the instructions in `docker/README.md' to create the Wagtail template.

Then, follow the instructions in the README file in the root of the newly created Wagtail project template.

Copy link
Member

@hancush hancush left a comment

Choose a reason for hiding this comment

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

Finally, a Wagtail cookiecutter! Looking good, @antidipyramid, though I spotted a few areas for improvement, particularly in the docs. Comments inline.

@fgregg
Copy link
Member

fgregg commented Mar 23, 2022

@antidipyramid , can you pull in the publish_docker_image.yml from the django cookie-cutter

@antidipyramid antidipyramid requested a review from hancush April 7, 2022 18:46
@hancush
Copy link
Member

hancush commented Apr 21, 2022

@antidipyramid Bumped a couple of threads and left some comments inline. Looks like a bit more work needed here... would you like my feedback on anything in the interim?

@antidipyramid antidipyramid requested a review from hancush May 5, 2022 13:27
Copy link
Member

@hancush hancush left a comment

Choose a reason for hiding this comment

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

Couple more little things inline... very close!

@antidipyramid antidipyramid requested a review from hancush May 19, 2022 16:39
Copy link
Member

@hancush hancush left a comment

Choose a reason for hiding this comment

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

Some small snafus. :-c

Comment on lines 52 to 55
Site.objects.all().delete()
Page.objects.all().delete()
PageRevision.objects.all().delete()
Image.objects.all().delete()
Copy link
Member

Choose a reason for hiding this comment

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

Removing these also removes the default Wagtail page, and since the content fixtures are empty, this causes an error on the admin site (and a 404 on the homepage). Is it necessary to flush these instances?

Copy link
Member

Choose a reason for hiding this comment

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

If we do want to flush existing content, how about checking whether there are pages in the fixture? If not, exit the command. Otherwise, we're safe to proceed with flushing and loading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since load_cms_content will be used anytime we can to update the default content, we do want to flush existing content before loading a Django dump. I think checking if initial_cms_content.json is empty is a great idea.

@antidipyramid antidipyramid requested a review from hancush June 2, 2022 18:43
Copy link
Member

@hancush hancush left a comment

Choose a reason for hiding this comment

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

Works as advertised. Thanks, @antidipyramid!

@derekeder
Copy link
Member

I get it when trying to run docker-compose run --rm cookiecutter -f new-django-app

@antidipyramid antidipyramid force-pushed the add-wagtail-cookiecutter branch from 23c102a to 2c27f98 Compare October 21, 2022 14:26
Copy link
Member

@derekeder derekeder left a comment

Choose a reason for hiding this comment

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

looks good to me - just made a few edits to the Readme with commands you need to run the first time you set it up.

I think we can bring this in!

@antidipyramid antidipyramid merged commit ab96e78 into main Nov 30, 2022
@derekeder
Copy link
Member

this is good to go. lets merge

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