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

Add edit_handler to AbstractMainMenu, AbstractFlatMenu #464 #468

Closed
wants to merge 1 commit into from

Conversation

yarickprih
Copy link
Contributor

Resolves #464

Extend AbstractMainMenu and AbstractFlatMenu with edit_handler attribute in order to support wagtail 5.2 requirements for non-Page models content tabs.

@yarickprih
Copy link
Contributor Author

yarickprih commented Nov 11, 2023

@ababic, the code coverage decreased 1% but i'm not exactly sure how to test edit_handler attributes. I've updated a single place in the tests where i though it would be appropriate to include edit_handler.
I'd be thankful If you could point or advice on that.

@yarickprih yarickprih changed the title Add edit_handler to AbstractMainMenu, AbstractFlatMenu Add edit_handler to AbstractMainMenu, AbstractFlatMenu #464 Nov 11, 2023
@ababic
Copy link
Collaborator

ababic commented Nov 11, 2023

Hi @yarickprih. These changes aren't actually needed, because the custom views used by wagtailmenus generate their own edit handler from the content_panels and settings_panels attributes on the model.

Adding these edit_handler attributes to the models actually means that, in order to add their own panels to either tab, developers need to completely override edit_handler instead of just adding items to content_panels and settings_panels (like they can currently).

@yarickprih
Copy link
Contributor Author

Hi @yarickprih. These changes aren't actually needed, because the custom views used by wagtailmenus generate their own edit handler from the content_panels and settings_panels attributes on the model.

Adding these edit_handler attributes to the models actually means that, in order to add their own panels to either tab, developers need to completely override edit_handler instead of just adding items to content_panels and settings_panels (like they can currently).

Yeah, didn't notice that part in the views. I was mainly aiming to get rid of wagtail warnings.
If this PR is not really required i'll close it

@ababic
Copy link
Collaborator

ababic commented Nov 11, 2023

Yeah, didn't notice that part in the views. I was mainly aiming to get rid of wagtail warnings.
If this PR is not really required i'll close it

Sorry @yarickprih, I was a little blunt in my previous reply. I haven't been involved in this project for a long while, so the notifications caused a bit of panic! It was awesome of you to hop onto this. But alas, I do think we need to approach this a little differently.

@ababic ababic closed this Nov 11, 2023
@yarickprih
Copy link
Contributor Author

yarickprih commented Nov 12, 2023

Yeah, didn't notice that part in the views. I was mainly aiming to get rid of wagtail warnings.
If this PR is not really required i'll close it

Sorry @yarickprih, I was a little blunt in my previous reply. I haven't been involved in this project for a long while, so the notifications caused a bit of panic! It was awesome of you to hop onto this. But alas, I do think we need to approach this a little differently.

Sorry for bothering @ababic, i have just a single question
who can i contact with regarding reviews/questions on this project (if you know ofc)?
i have 1 more small PR with translations that i'd like to be reviewed

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.

Wagtail 5.2 throws wagtailadmin.W002 warnings
2 participants