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

Fix classname deprecation warning in Wagtail 4.2 #74

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

chosak
Copy link
Member

@chosak chosak commented Aug 14, 2023

Wagtail 4.2 deprecates use of the class_name attribute in the icon tag.
This commit replaces it with classname instead. See:

https://docs.wagtail.org/en/stable/releases/4.2.html#adoption-of-classname-convention-for-some-template-tags-includes

@chosak chosak requested a review from willbarton August 14, 2023 19:45
@chosak
Copy link
Member Author

chosak commented Aug 14, 2023

The Wagtail 4.2 tests are failing here for the same reason as cfpb/wagtail-sharing#66; @willbarton should we bring over something similar to cfpb/wagtail-sharing#68 here?

@willbarton
Copy link
Member

@chosak Hmm. I think my preference would be to only test against LTS + latest supported version for Wagtail, because of the way their release cycle goes. This means 4.2 would be unnecessary to test against. It's a shame that release is broken, and unlikely to be fixed.

Wagtail-Flags doesn't already have its own separate test app, and it doesn't really need one, so we'd be adding one just to work around a problem upstream in an unsupported version of Wagtail.

I think my preference here is, if we need to test against 4.2, let's pin the last good release of 4.2 until we get to the 5.x LTS release as latest.

@willbarton
Copy link
Member

Actually — does Wagtail-Flags use anything from the test app in its tests? I don't see anything.

It's possible we copied boilerplate settings for tests that include it from other apps that need it.

@chosak
Copy link
Member Author

chosak commented Aug 15, 2023

Good point! I can open another PR to remove dependence on the testapp.

I do have another question, though, about this PR: if/when we merge this in, it'll mean this package is no longer compatible with Wagtail 4.1 and below. So the next release could be 4.2+ only. Or we could hold off merging this until such time as we want to drop support for those older versions; after all, this is only a deprecation warning, not broken functionality. What do you think?

@willbarton
Copy link
Member

Or we could hold off merging this until such time as we want to drop support for those older versions; after all, this is only a deprecation warning, not broken functionality. What do you think?

I think let's merge, but hold off on releasing until we're able to support the next LTS?

Wagtail 4.2 deprecates use of the `class_name` attribute in the icon tag.
This commit replaces it with `classname` instead. See:

https://docs.wagtail.org/en/stable/releases/4.2.html#adoption-of-classname-convention-for-some-template-tags-includes
@chosak chosak force-pushed the fix/class_name-to-classname branch from d896172 to 94fa56a Compare August 15, 2023 12:55
@chosak chosak merged commit 6520756 into main Aug 15, 2023
12 checks passed
@chosak chosak deleted the fix/class_name-to-classname branch August 15, 2023 12:57
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