Restore the Add "Other" button after bulk editing options#2616
Restore the Add "Other" button after bulk editing options#2616shervElmi wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2616 +/- ##
=========================================
Coverage 26.17% 26.18%
- Complexity 8831 8833 +2
=========================================
Files 144 144
Lines 29798 29809 +11
=========================================
+ Hits 7799 7804 +5
- Misses 21999 22005 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThe bulk options update flow now retrieves the options container element and determines if an "Other" option exists before updating its HTML. Upon successful modal close, the "Other" button is conditionally revealed if the option is present, using optional chaining for safe element access. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/src/admin/admin.js (1)
11095-11106: Make “Add Other” button re‑show logic depend on post‑update state as wellRight now
hasOtherOptionis computed from the DOM before you replaceinnerHTML, and you use it only to decide whether to re‑show#other_button_{fieldId}. If the returned HTML ever still includes an.frm_other_option, this would still unhide the “Add Other” button and allow users to add a second “Other” option.You can make this more robust by tracking both previous and current presence of the
.frm_other_optionafter the bulk update:
- Only re‑show the “Add Other” button when the field previously had an “Other” option and the updated HTML no longer does.
- Optionally hide the button when the updated HTML does contain an “Other” option, to preserve the “only one Other” invariant.
Here’s a minimal adjustment that stays within the current structure:
- success: function( html ) { - const optionsListEl = document.getElementById( 'frm_field_' + fieldId + '_opts' ); - const hasOtherOption = optionsListEl.querySelector( '.frm_single_option.frm_other_option' ); - optionsListEl.innerHTML = html; + success: function( html ) { + const optionsListEl = document.getElementById( 'frm_field_' + fieldId + '_opts' ); + const previouslyHadOther = !! optionsListEl?.querySelector( '.frm_single_option.frm_other_option' ); + + if ( optionsListEl ) { + optionsListEl.innerHTML = html; + } wp.hooks.doAction( 'frm_after_bulk_edit_opts', fieldId ); resetDisplayedOpts( fieldId ); if ( typeof modal !== 'undefined' ) { modal.dialog( 'close' ); document.getElementById( 'frm-update-bulk-opts' ).classList.remove( 'frm_loading_button' ); - if ( hasOtherOption ) { - document.getElementById( `other_button_${ fieldId }` )?.style.removeProperty( 'display' ); - } + const currentlyHasOther = !! optionsListEl?.querySelector( '.frm_single_option.frm_other_option' ); + const addOtherButton = document.getElementById( `other_button_${ fieldId }` ); + + if ( addOtherButton ) { + // Show button only when an "Other" was removed by this bulk update. + if ( previouslyHadOther && ! currentlyHasOther ) { + addOtherButton.style.removeProperty( 'display' ); + // Keep button hidden when an "Other" still exists. + } else if ( currentlyHasOther ) { + addOtherButton.style.display = 'none'; + } + } } }This keeps the behavior targeted to the “Other removed via bulk update” case and avoids exposing the “Add Other” button while an Other option is still present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/src/admin/admin.js (1)
js/admin/dom.js (3)
modal(12-109)modal(14-14)modal(607-607)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
|
Thanks @shervElmi! It looks like this was recently introduced with #2534. Truong commented on it, but it looks like it was merged anyway. Instead Abdi had logged a new issue for it https://github.com/Strategy11/formidable-pro/issues/6077. But it looks like it wasn't an issue until that update. It seems to work as expected in v6.25.1. I updated it in a commit on Abdi's PR for the option limiting update, but the issue was on master for a while. It should be fixed with #2620. I don't believe we need this PR anymore. |
2 participants
When editing radio button options using Bulk Edit or AI generation, the Add "Other" button would disappear and not come back. This fix restores the Add "Other" button visibility after updating options.
Related Issue
Fixes https://github.com/Strategy11/formidable-pro/issues/6092
Before
After