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

[7774] Make wagtail blocks available on different page types #884

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

hom3mad3
Copy link
Contributor

No description provided.

@hom3mad3 hom3mad3 changed the title WIP: [7774] Make blocks available on different page types [7774] Make blocks available on different page types Oct 26, 2023
@hom3mad3 hom3mad3 requested review from philli-m, goapunk and m4ra October 26, 2023 08:35
('video_block', apps_blocks.VideoBlock()),
('text_with_image', apps_blocks.TextWithImageBlock()),
('faq_accordion', apps_blocks.FaqBlock()),
('quote', apps_blocks.QuoteBlock()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is trailing comma a thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't throw any errors like with json (at least not in wagtail) but if I notice them I normally clean them up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so i think i will leave it, as i notice the entire code is like that

@philli-m philli-m self-assigned this Oct 26, 2023
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

looking good, I would just add the migration renames to the commits where files added and it's good to go

('video_block', apps_blocks.VideoBlock()),
('text_with_image', apps_blocks.TextWithImageBlock()),
('faq_accordion', apps_blocks.FaqBlock()),
('quote', apps_blocks.QuoteBlock()),
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't throw any errors like with json (at least not in wagtail) but if I notice them I normally clean them up

@@ -0,0 +1,47 @@
# Generated by Django 3.2.19 on 2023-10-25 17:36
Copy link
Contributor

Choose a reason for hiding this comment

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

although having a history of different named migrations locally is fine, when merging to main we should merge them with the correct name, you can use git rebase -i mainto add the name changes to the relevant commits so main only ever has a record of the correct name, does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philli-m updated respective commits with correct names.
can you please have a look and confirm if i got it right? 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

@hom3mad3 looks good! :)

@philli-m philli-m self-requested a review October 26, 2023 13:17
@philli-m philli-m removed their assignment Oct 26, 2023
@philli-m
Copy link
Contributor

philli-m commented Oct 26, 2023

@hom3mad3 also another fun thing about working with wagtail and it's constant need for migrations is that it's always a good idea to se if anything has been merged with migrations since you pulled, as the wagtail update just got merged you will need to delete your migrations you made for this pr and pull the most recent main and rebase and install and remake your migrations (sorry) this is because migrations on main must always be sequential and can't be doubled up as they always rely on previous one
oh and main now has a changelog folder so you can add a change log file, sorry forgot that before :(

@hom3mad3 hom3mad3 force-pushed the mr-2023-10-wagtail-blocks branch from 2935fe4 to 86b4f5e Compare October 26, 2023 14:38
@hom3mad3 hom3mad3 force-pushed the mr-2023-10-wagtail-blocks branch from c90405e to e0bf1d3 Compare October 26, 2023 15:21
@hom3mad3 hom3mad3 changed the title [7774] Make blocks available on different page types [7774] Make wagtail blocks available on different page types Oct 26, 2023
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

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

great! :)

@philli-m philli-m merged commit 3b8919f into main Oct 26, 2023
2 checks passed
@philli-m philli-m deleted the mr-2023-10-wagtail-blocks branch October 26, 2023 15:38
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.

2 participants