Conversation
WalkthroughThis pull request refactors the payment gateway settings UI in the admin panel by introducing a client-side accordion component that groups PayPal, Bank Transfer, and Credit Card gateway options. It scans the existing payment table, reorganizes rows by gateway type, injects dynamic trigger rows with toggle behavior, adds conditional PRO-gated features for Stripe, and injects styling and PRO badge indicators. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/js/admin/settings.js (1)
62-74:⚠️ Potential issue | 🟠 Major
allowGatewaysTrre-surfaces after every search reset.
allowGatewaysTris hidden using an inline style at Line 127 (allowGatewaysTr.style.display = 'none'), butwpuf_search_reset()at Line 70–72 unconditionally callsrow.style.display = ''on everytrin the metabox holder, clearing that inline style and making the original gateway-checkboxes row visible again alongside the newly injected accordion UI.Fix: use a CSS class to hide the row — inline styles are easily clobbered by the reset.
🐛 Proposed fix
Add the CSS class alongside the injected
<style>block (Lines 158–218):+ .wpuf-accordion-hidden { + display: none !important; + }Then replace the inline-style hide:
- allowGatewaysTr.style.display = 'none'; + allowGatewaysTr.classList.add('wpuf-accordion-hidden');Also applies to: 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/admin/settings.js` around lines 62 - 74, The reset routine wpuf_search_reset currently clears inline styles on all tr elements which re-shows the gateway row; stop using inline styles and instead add a dedicated CSS class (e.g., "wpuf-hidden-gateway") to hide allowGatewaysTr when injecting the accordion, and update the injected <style> block to include .wpuf-hidden-gateway { display: none !important; } ; replace the code that sets allowGatewaysTr.style.display = 'none' with adding that class (allowGatewaysTr.classList.add('wpuf-hidden-gateway')), and remove any code that later tries to clear inline display so wpuf_search_reset can safely reset rows without re-surfacing the gateway row.
🧹 Nitpick comments (3)
assets/js/admin/settings.js (3)
293-293: Hardcoded upgrade URL should be passed viawp_localize_script.
https://wedevs.com/wp-user-frontend-pro/is hardcoded in JavaScript. Consider passing it from PHP usingwp_localize_script(alongside any other JS config) so the URL can be controlled server-side without touching the JS bundle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/admin/settings.js` at line 293, The hardcoded upgrade URL inside the assets/js/admin/settings.js string (see the dummyTd.innerHTML assignment) should be replaced with a value provided from PHP via wp_localize_script (or wp_add_inline_script) so the URL can be configured server-side; update the PHP that enqueues this script to pass an array (e.g., ['upgradeUrl' => 'https://...']) to the script handle, then modify the JS to read the localized object (e.g., window.<yourLocalizedObject>.upgradeUrl) instead of the literal "https://wedevs.com/wp-user-frontend-pro/" in the dummyTd.innerHTML construction.
307-365: Accordion button missingaria-expanded(andaria-controls) attributes.
aria-expandedindicates whether a control is expanded or collapsed and should be applied to the focusable, interactive control that toggles the visibility of the object. When the content is visible, the element with role button hasaria-expandedset totrue; when hidden, it is set tofalse. Without this, screen readers have no way to announce the open/closed state of each gateway panel.♻️ Proposed fix
triggerBtn.type = 'button'; triggerBtn.className = 'wpuf-accordion-trigger'; triggerBtn.setAttribute('data-target-group', key); + triggerBtn.setAttribute('aria-expanded', 'false');Then in the click handler, sync the attribute:
- this.classList.add('active'); + this.classList.add('active'); + this.setAttribute('aria-expanded', 'true'); document.querySelectorAll('tr[data-group="' + key + '"]').forEach(function (tr) { tr.classList.add('active'); });And when closing all accordions:
document.querySelectorAll('.wpuf-accordion-trigger').forEach(function (btn) { btn.classList.remove('active'); + btn.setAttribute('aria-expanded', 'false'); var g = btn.getAttribute('data-target-group');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/admin/settings.js` around lines 307 - 365, The accordion trigger (triggerBtn) lacks accessibility attributes; set aria-expanded="false" on creation and set aria-controls to the id of the rows group (generate a stable id using the group key, e.g. "wpuf-accordion-" + key) so screen readers can associate the button with its content; ensure each row in group.rows gets that same container id or a wrapper element id to reference; in the click handler for triggerBtn (and in the routine that closes all accordions where you remove the 'active' class from .wpuf-accordion-trigger), update triggerBtn.setAttribute('aria-expanded', 'true'|'false') accordingly and when toggling the group update the referenced content's aria-hidden to match the visibility (true when hidden, false when shown) so aria-expanded and aria-controls stay in sync with the visible state.
182-196:float: rightis a no-op inside a flex container.
.wpuf-accordion-triggerusesdisplay: flex, sofloat: righton the::afterpseudo-element (Line 190) has no effect — the arrow is already pushed to the right bymargin-left: autoon Line 191. Thefloatdeclaration can be removed to avoid confusion.♻️ Proposed fix
.wpuf-accordion-trigger:after { content: ''; display: block; width: 8px; height: 8px; border-right: 2px solid `#777`; border-bottom: 2px solid `#777`; transform: translateY(-2px) rotate(45deg); - float: right; margin-left: auto; transition: transform 0.3s ease; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/admin/settings.js` around lines 182 - 196, Remove the redundant float declaration from the pseudo-element rule: in the .wpuf-accordion-trigger:after CSS block remove "float: right" (the element is inside a flex container and already positioned by margin-left: auto), leaving the rest of the styles (width, height, borders, transform, margin-left: auto, transition) unchanged; ensure the active state .wpuf-accordion-trigger.active:after remains as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/js/admin/settings.js`:
- Around line 9-36: The search handler currently sets tr.style.display =
'table-row' for matches using the static NodeList "content", which causes
payment rows (originally hidden by the accordion CSS) to leak into view; to fix,
add a guard like the existing 'wpuf_mails' block for content_id ===
'wpuf_payment' that resets display on its tbody rows (e.g.,
querySelectorAll('tr').forEach(tr => tr.style.display = '')) so payment rows
remain controlled by the accordion; apply this same guard in both places where
the content iteration runs (the search_input keyup handler and the repeated loop
later around the same logic) and keep using the same identifiers: search_input,
content, content_id, value_tab, and 'wpuf_payment'.
- Around line 225-242: The PRO icon resolution is doing a redundant DOM query
and using a fragile split to derive base URLs; update the proIconImg lookup to
only try document.querySelector('.pro-icon img') once (remove the redundant
document.querySelector('span.pro-icon img') fallback) and, when deriving baseUrl
from anyImg.src in the anyImg branch, compute baseUrl using the last occurrence
of "/assets/" (e.g., use src.substring(0, src.lastIndexOf('/assets/')) ) before
building existingProIconSrc; ensure you still fall back to fallbackProIconSvg if
no anyImg is found and keep references to proIconImg, anyImg,
existingProIconSrc, and fallbackProIconSvg unchanged.
---
Outside diff comments:
In `@assets/js/admin/settings.js`:
- Around line 62-74: The reset routine wpuf_search_reset currently clears inline
styles on all tr elements which re-shows the gateway row; stop using inline
styles and instead add a dedicated CSS class (e.g., "wpuf-hidden-gateway") to
hide allowGatewaysTr when injecting the accordion, and update the injected
<style> block to include .wpuf-hidden-gateway { display: none !important; } ;
replace the code that sets allowGatewaysTr.style.display = 'none' with adding
that class (allowGatewaysTr.classList.add('wpuf-hidden-gateway')), and remove
any code that later tries to clear inline display so wpuf_search_reset can
safely reset rows without re-surfacing the gateway row.
---
Nitpick comments:
In `@assets/js/admin/settings.js`:
- Line 293: The hardcoded upgrade URL inside the assets/js/admin/settings.js
string (see the dummyTd.innerHTML assignment) should be replaced with a value
provided from PHP via wp_localize_script (or wp_add_inline_script) so the URL
can be configured server-side; update the PHP that enqueues this script to pass
an array (e.g., ['upgradeUrl' => 'https://...']) to the script handle, then
modify the JS to read the localized object (e.g.,
window.<yourLocalizedObject>.upgradeUrl) instead of the literal
"https://wedevs.com/wp-user-frontend-pro/" in the dummyTd.innerHTML
construction.
- Around line 307-365: The accordion trigger (triggerBtn) lacks accessibility
attributes; set aria-expanded="false" on creation and set aria-controls to the
id of the rows group (generate a stable id using the group key, e.g.
"wpuf-accordion-" + key) so screen readers can associate the button with its
content; ensure each row in group.rows gets that same container id or a wrapper
element id to reference; in the click handler for triggerBtn (and in the routine
that closes all accordions where you remove the 'active' class from
.wpuf-accordion-trigger), update triggerBtn.setAttribute('aria-expanded',
'true'|'false') accordingly and when toggling the group update the referenced
content's aria-hidden to match the visibility (true when hidden, false when
shown) so aria-expanded and aria-controls stay in sync with the visible state.
- Around line 182-196: Remove the redundant float declaration from the
pseudo-element rule: in the .wpuf-accordion-trigger:after CSS block remove
"float: right" (the element is inside a flex container and already positioned by
margin-left: auto), leaving the rest of the styles (width, height, borders,
transform, margin-left: auto, transition) unchanged; ensure the active state
.wpuf-accordion-trigger.active:after remains as-is.
| search_input.addEventListener('keyup', function (e) { | ||
| var search_value = e.target.value.toLowerCase(); | ||
| var value_tab = []; | ||
| var value_tab = []; | ||
|
|
||
| if ( search_value.length ) { | ||
| if (search_value.length) { | ||
| close.style.display = 'flex'; | ||
| content.forEach(function (row, index) { | ||
|
|
||
| var content_id = row.closest('div').getAttribute('id'); | ||
| var tab_id = content_id + '-tab'; | ||
| var found_value = row.innerText.toLowerCase().includes( search_value ); | ||
| var tab_id = content_id + '-tab'; | ||
| var found_value = row.innerText.toLowerCase().includes(search_value); | ||
|
|
||
| if ( found_value ){ | ||
| if (found_value) { | ||
| row.closest('tr').style.display = 'table-row'; | ||
| }else { | ||
| } else { | ||
| row.closest('tr').style.display = 'none'; | ||
| } | ||
|
|
||
| if ( 'wpuf_mails' === content_id ){ | ||
| if ('wpuf_mails' === content_id) { | ||
| row.closest('tbody').querySelectorAll('tr').forEach(function (tr) { | ||
| tr.style.display = ''; | ||
| }); | ||
| } | ||
|
|
||
| if ( found_value === true && ! value_tab.includes( tab_id ) ) { | ||
| if (found_value === true && !value_tab.includes(tab_id)) { | ||
| value_tab.push(tab_id); | ||
| } | ||
| }) |
There was a problem hiding this comment.
Search overrides accordion display:none for payment rows.
content is a static NodeList captured at Line 4 before the accordion reorganizes the DOM. The original payment table rows (PayPal settings, Bank Transfer settings, Stripe settings) are part of that snapshot. When a user types a matching term, Lines 21–25 apply tr.style.display = 'table-row', which overrides .wpuf-accordion-row { display: none } — making those rows appear outside their accordion context. The accordion trigger rows are NOT in content (they are created after Line 4), so they won't be shown alongside the leaked content rows.
Lines 27–31 already have a precedent for special-casing a tab; the same pattern is needed for wpuf_payment.
🐛 Proposed fix — add a payment-tab guard parallel to the `wpuf_mails` guard
if ('wpuf_mails' === content_id) {
row.closest('tbody').querySelectorAll('tr').forEach(function (tr) {
tr.style.display = '';
});
}
+
+ if ('wpuf_payment' === content_id) {
+ // Let the accordion handle its own row visibility; skip inline-style overrides
+ row.closest('tr').style.display = '';
+ }Also applies to: 27-31, 130-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/js/admin/settings.js` around lines 9 - 36, The search handler
currently sets tr.style.display = 'table-row' for matches using the static
NodeList "content", which causes payment rows (originally hidden by the
accordion CSS) to leak into view; to fix, add a guard like the existing
'wpuf_mails' block for content_id === 'wpuf_payment' that resets display on its
tbody rows (e.g., querySelectorAll('tr').forEach(tr => tr.style.display = ''))
so payment rows remain controlled by the accordion; apply this same guard in
both places where the content iteration runs (the search_input keyup handler and
the repeated loop later around the same logic) and keep using the same
identifiers: search_input, content, content_id, value_tab, and 'wpuf_payment'.
| var existingProIconSrc = ''; | ||
| var proIconImg = document.querySelector('.pro-icon img'); | ||
| if (!proIconImg) { | ||
| // Also try settings fields that may be hidden | ||
| proIconImg = document.querySelector('span.pro-icon img'); | ||
| } | ||
| if (proIconImg) { | ||
| existingProIconSrc = '<img src="' + proIconImg.getAttribute('src') + '" alt="PRO" style="vertical-align:middle;">'; | ||
| } else { | ||
| // Derive path from any known plugin img on the page, or use absolute path pattern | ||
| var anyImg = document.querySelector('img[src*="wp-user-frontend"][src*="assets"]'); | ||
| if (anyImg) { | ||
| var baseUrl = anyImg.getAttribute('src').split('/assets/')[0]; | ||
| existingProIconSrc = '<img src="' + baseUrl + '/assets/images/pro-badge.svg" alt="PRO" style="vertical-align:middle;">'; | ||
| } else { | ||
| existingProIconSrc = fallbackProIconSvg; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fragile path derivation and redundant query in PRO icon resolution.
Two minor concerns:
-
Redundant query (Line 229):
document.querySelector('span.pro-icon img')is a strict subset of.pro-icon imgalready tried on Line 226. If Line 226 returnsnull, Line 229 never returns a different result. -
Fragile
.split('/assets/')[0](Line 237): If any image'ssrcURL contains/assets/in the subdomain or path prefix (e.g.,https://cdn-assets.domain.com/plugins/wp-user-frontend/assets/img.png), the split yields an incorrect base. Prefersrc.substring(0, src.lastIndexOf('/assets/'))to match the last occurrence, which is more reliable for typical WP plugin URL structures.
♻️ Proposed fix
- if (!proIconImg) {
- // Also try settings fields that may be hidden
- proIconImg = document.querySelector('span.pro-icon img');
- }
if (proIconImg) {- var baseUrl = anyImg.getAttribute('src').split('/assets/')[0];
+ var imgSrc = anyImg.getAttribute('src');
+ var baseUrl = imgSrc.substring(0, imgSrc.lastIndexOf('/assets/'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/js/admin/settings.js` around lines 225 - 242, The PRO icon resolution
is doing a redundant DOM query and using a fragile split to derive base URLs;
update the proIconImg lookup to only try document.querySelector('.pro-icon img')
once (remove the redundant document.querySelector('span.pro-icon img') fallback)
and, when deriving baseUrl from anyImg.src in the anyImg branch, compute baseUrl
using the last occurrence of "/assets/" (e.g., use src.substring(0,
src.lastIndexOf('/assets/')) ) before building existingProIconSrc; ensure you
still fall back to fallbackProIconSvg if no anyImg is found and keep references
to proIconImg, anyImg, existingProIconSrc, and fallbackProIconSvg unchanged.
|
Pro badge is showing, though Pro is off. @anik-fahmid vai |
|
duplicate #1821 |
2 participants
feat(payment): redesign payment gateway settings page
Summary by CodeRabbit
New Features
Style