-
Notifications
You must be signed in to change notification settings - Fork 21
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 page chooser #85
Fix page chooser #85
Conversation
`chooserUrls.pageChooser` has been removed in Wagtail 6.1
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
=======================================
Coverage 88.44% 88.44%
=======================================
Files 15 15
Lines 753 753
Branches 140 140
=======================================
Hits 666 666
Misses 51 51
Partials 36 36 ☔ View full report in Codecov by Sentry. |
{% url 'wagtailadmin_choose_page' as wagtailadmin_choose_page_url %} | ||
window.AB_TESTING_CHOOSE_PAGE_URL = '{{ wagtailadmin_choose_page_url|escapejs }}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather see us change this to something that is inherently compatible with strict content security policies.
@mgax could you change this to use json_script
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that the patten on the above two lines is okay, i.e. setting the value on wagtailConfig
? If not, I'd better change that one too.
wagtail-ab-testing/wagtail_ab_testing/templates/wagtail_ab_testing/add_form.html
Lines 35 to 36 in cbeff6f
{% url 'wagtailadmin_home' as wagtailadmin_root_url %} | |
wagtailConfig.ADMIN_ROOT_URL = '{{ wagtailadmin_root_url|escapejs }}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, changing that too would make sense now that I look at it more closely!
Checking its usage, it appears to only be used to construct a URL to fetch page information:
wagtail-ab-testing/wagtail_ab_testing/static_src/components/GoalSelector/index.tsx
Line 40 in 7e5bb95
`${wagtailConfig.ADMIN_ROOT_URL}api/main/pages/${selectedPageId}/`, |
ADMIN_ROOT_URL
is internal to us, there is no reference inside the (current) Wagtail codebase. No need to put this in wagtailConfig
I feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now.
I'm having a hard time fixing the frontend lint error (https://github.com/wagtail-nest/wagtail-ab-testing/actions/runs/10353044821/job/28654997687?pr=85). If I run this locally: npm ci
npm run lint I don't get any errors. I've confirmed that I'm running Node 20. |
``` ./node_modules/.bin/prettier --write 'wagtail_ab_testing/**/*.{ts,tsx,scss}' ```
Fixed with help from @siimonevans (thanks!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Alex! I'm happy with this 👍
Refs. #84 (comment)
chooserUrls.pageChooser
has been removed in Wagtail 6.1: https://docs.wagtail.org/en/stable/releases/6.1.html#deprecation-of-window-chooserurls-within-draftail-choosers