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

Allow wagtail 4 #42

Merged
merged 6 commits into from
Oct 14, 2022
Merged

Allow wagtail 4 #42

merged 6 commits into from
Oct 14, 2022

Conversation

nickmoreton
Copy link
Contributor

@nickmoreton nickmoreton commented Sep 23, 2022

This allows for Wagtail v4

Support ticket: https://projects.torchbox.com/projects/support-team/tickets/591

Adjustments were required in Javascript and templates to pick up new css selectors and styling.

@nickmoreton nickmoreton marked this pull request as ready for review September 23, 2022 13:28
@nickmoreton
Copy link
Contributor Author

8fd956e is to bring back the template and javascript file that works with 2.15, 2.16 & 3.0

from wagtail import VERSION as WAGTAIL_VERSION

if WAGTAIL_VERSION >= (4, 0):
template_name = "wagtail_footnotes/admin/footnotes_modals.html"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a typo here, it should be singular "footnotes_modal.html".

Copy link
Contributor

Choose a reason for hiding this comment

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

PR to fix this available in #44

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsma Thanks for doing that 👍

@jsma
Copy link
Contributor

jsma commented Oct 5, 2022

Thanks for putting this together @nickmoreton!

I've tested this with Wagtail 4 and we now have it running in production (with the patch to fix the template name typo). I unfortunately do not have time to test older versions of Wagtail.

I do want to note that there are UI/UX issues in Wagtail 4 where it is nearly impossible to add a footnote using the new hidden-by-default rich text toolbar. I've reported the basic issue upstream since this isn't really fixable in wagtail-footnotes, but wanted to detail the specifics here.

Prior to Wagtail v4, it was possible to drop the cursor in the correct spot and simply select "Fn" from the always visible toolbar. With Wagtail v4, this workflow is now impossible, the toolbar is not activated:

Screen Shot 2022-10-04 at 12 28 52 PM

As far as I know, it's only possible to make the toolbar visible after selecting something. If you select text, the toolbar appears but of course this will replace the selected word:

Screen Shot 2022-10-04 at 12 29 22 PM

The only workaround I've found is to highlight a space:

Screen Shot 2022-10-04 at 12 49 09 PM

This is not "discoverable" by non-tech editors. I only figured this out by watching the animated GIF in the "Extending the Draftail Editor" docs demonstrating a stock chooser. It also requires adding another space to replace the space replaced by the footnote reference when inserting footnotes mid-sentence.

Hopefully this can be addressed upstream but in the meantime, should we add some documentation/screen shots to the README to make this more obvious?

@nickmoreton
Copy link
Contributor Author

..., should we add some documentation/screen shots to the README to make this more obvious?

That sounds like a good solution for the moment. Your screen shots here are great, may be use those in the README. It could be pictorial introduction to the package. The third one shows exactly what you need to do.

@2qxtx
Copy link

2qxtx commented Oct 13, 2022

When can we expect this pull request to be included in the next release? Don't mean to rush the process - just trying to gauge how long it might take before we can start using this package with Wagtail 4. Thank you.

@zerolab
Copy link
Member

zerolab commented Oct 13, 2022

@2qxtx I am sorry it is taking a while. You could pip install off this branch to continue work.
Will try and see if we can get some time to do a new release.

@nickmoreton
Copy link
Contributor Author

I've manually tested this on all Wagtail versions from 2.15 to 4.0.2 and all works as expected.

It would be good to get some tests in place: #46

Also we should monitor this: #42 (comment) that has an upstream issue open: wagtail/wagtail#9281

@nickmoreton nickmoreton merged commit 3f13575 into main Oct 14, 2022
@nickmoreton nickmoreton deleted the support/wagtail-4 branch October 14, 2022 15:35
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