-
Notifications
You must be signed in to change notification settings - Fork 91
gpnf-sortable-entries.php
: Fixed an issue with sortable entries not working correctly.
#1087
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
base: master
Are you sure you want to change the base?
Conversation
… working correctly.
WalkthroughThe JavaScript event handler for the Changes
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gp-nested-forms/gpnf-sortable-entries.php (1)
286-286
: Consider removing the debug console.log statementThere's a
console.log(gpnf)
statement that was likely used for debugging during development. It's best to remove this before moving to production.- console.log(gpnf)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gp-nested-forms/gpnf-sortable-entries.php
(1 hunks)
🔇 Additional comments (2)
gp-nested-forms/gpnf-sortable-entries.php (2)
239-242
: Great fix for enabling sorting on all forms!By adding a check to see if
self.formId
is truthy before comparing it togpnf.formId
, you've ensured that when the sortable entries feature is configured to work on all forms (whenself.formId
is false), the code won't return early. This effectively fixes the issue where the snippet only worked for a specific form configuration.
244-245
: Correctly updated selectors to use dynamic form and field IDsThese changes ensure that the jQuery selectors use the dynamic IDs from the
gpnf
object rather than static values fromself
. This is essential for making the feature work across all forms and fields, especially whenself.formId
andself.fieldId
are falsy in the "all forms/fields" configuration.
Context
⛑️ Ticket(s): https://secure.helpscout.net/conversation/2918586555/82589
Summary
The snippet doesn't appear to work when applied to all forms and also all nested forms field on a form. It only works when the configuration is set to apply it to a specific form and specific Nested Forms field.
Loom of the issue (Samuel):
https://www.loom.com/share/bff4f13258cd4ddaa8fc24e7819558b7
After the update:
https://www.loom.com/share/837b523b30ad495c945a2e976fd896ac