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

Adopt classname instead of classnames in all MenuItem/Sidebar usage #10907

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

lb-
Copy link
Member

@lb- lb- commented Sep 17, 2023

In Wagtail 4.2 we started being more consistent with how we support classname kwargs in various components and templates, this seemed to be received well and means our code is more predictable.

At the time we avoided changing anything that was not yet fully documented or just had a small surface area. In this PR I propose we adjust one documented use case MenuItem (and related) with a clear deprecation path.

I think this will make using Wagtail more intuitive for developers and sets us up for easier customisation in the long term.

Details

Checklist

  • Do the tests still pass?
  • Does the code comply with the style guide?
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For front-end changes: Did you test on all of Wagtail’s supported environments?
  • For new features: Has the documentation been updated accordingly?

@squash-labs
Copy link

squash-labs bot commented Sep 17, 2023

Manage this branch in Squash

Test this branch here: https://lb-cleanupadopt-classname-menu-9y7xb.squash.io

@lb- lb- force-pushed the cleanup/adopt-classname-menu-items branch from 366b084 to dd73aa4 Compare September 25, 2023 19:46
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Lotf of great stuff here, @lb- 👏🏼
Left a number of suggestions

docs/releases/5.2.md Outdated Show resolved Hide resolved
docs/releases/5.2.md Outdated Show resolved Hide resolved
docs/releases/5.2.md Outdated Show resolved Hide resolved
docs/releases/5.2.md Outdated Show resolved Hide resolved
wagtail/contrib/settings/registry.py Outdated Show resolved Hide resolved
@lb- lb- force-pushed the cleanup/adopt-classname-menu-items branch from 41b88e4 to fee562c Compare October 2, 2023 09:08
docs/releases/5.2.md Outdated Show resolved Hide resolved
@lb- lb- force-pushed the cleanup/adopt-classname-menu-items branch from fee562c to d006beb Compare October 2, 2023 09:15
@lb-
Copy link
Member Author

lb- commented Oct 2, 2023

Thanks @zerolab - ready for another round if you have time.

Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

This is so close. One minor suggestion.

Also 2 other points:

  1. I know modeladmin is being deprecated, but it is worth updating it as well since ModelAdminMenuItem subclasses MenuItem and passes classnames in __init__ - https://github.com/wagtail/wagtail/blob/main/wagtail/contrib/modeladmin/menus.py#L24 (also L52)

  2. One to address in a follow-up PR, I think - images.formats.Format that uses classnames as an arg

wagtail/admin/ui/sidebar.py Outdated Show resolved Hide resolved
@lb- lb- force-pushed the cleanup/adopt-classname-menu-items branch from d006beb to 05266a5 Compare October 2, 2023 21:12
@lb-
Copy link
Member Author

lb- commented Oct 2, 2023

Thanks @zerolab fixed up now. Inc. modeladmin items.

@lb- lb- requested a review from zerolab October 2, 2023 21:20
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

I just realised that we don't have tests for the new RemovedInWagtail60Warning

We have a number of them for other areas -
https://github.com/search?q=repo%3Awagtail%2Fwagtail+RemovedInWagtail60Warning+path%3A%2Ftests\%2F%2F&type=code - and I am torn.

Other than this, LGTM. Epic tidy up

@lb- lb- force-pushed the cleanup/adopt-classname-menu-items branch from 05266a5 to b822f34 Compare October 2, 2023 22:18
@lb-
Copy link
Member Author

lb- commented Oct 2, 2023

OK I have add some tests for the core MenuItem & LinkMenuItem only (instead of all the other classes) if that's OK & push up with changelog added. If all good we can merge. Otherwise let me know if I should add deprecation warning tests for the full suite of all menu item related classes.

Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

:shipit:

- Adds a deprecation path, including documentation to further remove unpredictable naming of adding `classname` in Python APIs
- Intentionally keeps `classNames` in Telepath adaptor inner usage as this convention is not set, however, the server side value passed in will use `classname`
- See https://docs.wagtail.org/en/stable/contributing/general_guidelines.html#use-classname-in-python-html-template-tag-variables
- See wagtail#9769 & wagtail#9770
@lb- lb- force-pushed the cleanup/adopt-classname-menu-items branch from b822f34 to a4c7613 Compare October 3, 2023 20:49
@lb-
Copy link
Member Author

lb- commented Oct 3, 2023

Thanks @zerolab - I really appreciate the reviews.

@lb- lb- merged commit 76a0c49 into wagtail:main Oct 3, 2023
19 checks passed
@lb- lb- deleted the cleanup/adopt-classname-menu-items branch October 3, 2023 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:Menu Page tree menu, sidebar, locale menu type:Cleanup/Optimisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants