-
Notifications
You must be signed in to change notification settings - Fork 22
Add confirmation page for deleting A/B tests associated with a page #94
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 confirmation page for deleting A/B tests associated with a page #94
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 93.51% 93.78% +0.26%
==========================================
Files 42 43 +1
Lines 1620 1690 +70
Branches 107 111 +4
==========================================
+ Hits 1515 1585 +70
Misses 69 69
Partials 36 36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mgax
left a comment
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.
Hi @joelwilliam2005, thanks for fixing this issue, and taking the time to make a PR! 🙌
I've tested the branch locally and it does what it says. The UI is nice and slick!
However, it does not address deleting pages from their parent page listing (which is what I tried initially, when the AB test was still active, since I did not have access to its individual delete button). Since that is a bulk delete action, I'm not sure what to do about it.
wagtail_ab_testing/templates/wagtail_ab_testing/confirm_delete_ab_tests.html
Outdated
Show resolved
Hide resolved
@mgax I have fixed the addressed issues. |
mgax
left a comment
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.
Lots of good stuff in here, nice work @joelwilliam2005! I've left some suggestions, hopefully I'm not overwhelming you with change requests 😅
Sorry about the code style nitpicks; ideally we should have automated linting in place to catch them. I've opened a ticket for this: #95.
wagtail_ab_testing/templates/wagtail_ab_testing/confirm_delete_ab_tests.html
Outdated
Show resolved
Hide resolved
wagtail_ab_testing/templates/wagtail_ab_testing/confirm_delete_ab_tests.html
Outdated
Show resolved
Hide resolved
wagtail_ab_testing/templates/wagtail_ab_testing/confirm_delete_ab_tests.html
Outdated
Show resolved
Hide resolved
wagtail_ab_testing/templates/wagtail_ab_testing/confirm_delete_ab_tests.html
Outdated
Show resolved
Hide resolved
wagtail_ab_testing/templates/wagtail_ab_testing/confirm_delete_ab_tests.html
Outdated
Show resolved
Hide resolved
|
Thank you for reviewing @mgax |
mgax
left a comment
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.
Hi @joelwilliam2005, thanks for addressing the comments! The PR is in pretty good shape I think. There are a couple of issues left to address. Cheers!
e43931a to
07bafd9
Compare
|
Sorry, I've pushed an erroneous commit to your branch, I've now removed it. |
mgax
left a comment
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.
@joelwilliam2005 nice work, thank you! 🎉
|
@joelwilliam2005 @mgax wanted to let you know that I just released v0.12 with this confirmation screen included 🎉 Thank you both for your work on this! |
Fixes issue #90
By adding new Confirmation page for deleting A/B tests associated with a page.