BTI-778 restructure WooCommerce plugin menu & settings for improved usability - #523
Conversation
…e-plugin-menu-&-settings-for-improved-usability # Conflicts: # index.php # library/css/buckaroo-custom.css
…plugin-menu-&-settings-for-improved-usability' into BTI-778-Restructure-WooCommerce-plugin-menu-&-settings-for-improved-usability
vildanbina
left a comment
vildanbina
left a comment
There was a problem hiding this comment.
three things not tied to a single line.
the country and currency badges were added to every gateway, but only about 20 got a method_description. these did not: Eps, Blik, MbWay, Multibanco, PayByBank, Payconiq, GiftCard, Wero, WeChatPay, and the five single card gateways. for those the card falls back to getPaymentDescription(), which returns whatever the merchant typed as their checkout description. so half the cards show Buckaroo copy and half show the shop's own text. either write the missing ones, or leave the line out when there is no description.
readme.txt loses 424 lines, the whole changelog from 3.5.0 down. deliberate trim for the wp.org page, or something a merge ate?
nit: index.php now says "Buckaroo Payments for WooCommerce" but readme.txt still starts with === Buckaroo Woocommerce Payments Plugin ===. wp.org takes the title shown in the plugin directory from readme.txt, so the rename would not actually show up there.
| <script> | ||
| (function () { | ||
| document.addEventListener('DOMContentLoaded', function () { | ||
| var allLinks = document.querySelectorAll('#wpbody-content a, .woocommerce-layout__header-back-link, .woocommerce-navigation-back-button'); |
There was a problem hiding this comment.
this script grabs every link inside #wpbody-content that has tab=checkout in the href but no section=. two of those are links you do not want to touch:
- the "Return to payments" link WC prints above the settings form
- the Payments tab in the settings nav
both point at admin.php?page=wc-settings&tab=checkout, so both get replaced with history.back().
why that is a problem: history.back() goes to whatever the browser saw last, which is not always the payments list. if the merchant opened this page in a new tab or from a bookmark there is no previous page, so the link does nothing and looks broken. if they came from the Buckaroo settings page, back sends them there instead of to the payments list.
the hrefs already point at the right page. what is this working around? if it is about keeping scroll position or a filter, there is likely a way to do that without overriding the links.
| value="<?php echo esc_attr($merchant_key); ?>" | ||
| class="input-text regular-input" | ||
| placeholder="<?php esc_attr_e('Enter your website key', 'wc-buckaroo-bpe-gateway'); ?>" | ||
| autocomplete="off" /> |
There was a problem hiding this comment.
on develop both keys are declared as required:
// PaymentMethodSettings.php:52
'custom_attributes' => ['required' => 'required'],these inputs are written by hand and do not have that attribute, so the browser no longer blocks saving when the field is empty. nothing checks it on save either. a merchant can save the page with no credentials and get no warning, then payments fail later with nothing pointing at the cause.
you already read custom_attributes off form_fields for the two buttons further down. do the same for these two inputs and required comes back for free.
also the old field description had a link straight to the Plaza page where the keys live. that link is gone now, worth keeping it in the help text.
| $this->id = 'buckaroo_klarnakp'; | ||
| $this->title = 'Klarna: Pay later'; | ||
| $this->method_title = 'Buckaroo Klarna Pay later (authorize/capture)'; | ||
| $this->method_description = __('Klarna pay-after-delivery, authorized at checkout and captured on shipment.', 'wc-buckaroo-bpe-gateway'); |
There was a problem hiding this comment.
this line gets overwritten a few lines below.
line 32 calls parent::__construct(), which is KlarnaGateway::__construct(), and that sets method_description again on KlarnaGateway.php:23 with the Merchant of Record text. php runs it in that order, so the KP settings page ends up showing the MoR description instead of yours.
two ways to fix it: move this line below parent::__construct(), or take the assignment out of KlarnaGateway and let each subclass set its own.
|
|
||
| public bool $capturable = false; | ||
|
|
||
| public $method_description = ''; |
There was a problem hiding this comment.
two things here.
first, WC_Payment_Gateway already declares public $method_description = '' (abstract-wc-payment-gateway.php:96), so this line adds nothing. it is inherited.
second, and this one is visible to merchants: WC_Payment_Gateway::admin_options() already prints the description above the form with wpautop(get_method_description()). the new summary card prints the same text again at line 476. so every gateway settings page now shows the description twice, once as a loose paragraph and once inside the card.
if the card is where you want it, keep the copy in a property of your own instead of method_description. then WC has nothing to print and only the card shows it.
|
|
||
| echo '</tr>'; | ||
| ?> | ||
| <div class="buckaroo-payment-card buckaroo-gateway-list__card" data-gateway_id="<?php echo esc_attr($gateway->id); ?>"> |
There was a problem hiding this comment.
this card and the one in render_payment_list() at line 668 are the same html. same icon block with the same placeholder fallback, same title, same subtitle, same status pill, and the same settings svg written out in full in both places. the only real differences are the extra buckaroo-gateway-list__card class and the toggle link.
that means every change to the card has to be made twice, and one copy will eventually be forgotten.
one private method would cover both, something like render_gateway_card($gateway, $withToggle = false), plus a small one for the svg.
| echo '<div class="bk-gateway-summary-card__info">'; | ||
| echo '<div class="bk-gateway-summary-card__title">' . esc_html($display_title) . '</div>'; | ||
| echo '<div class="bk-gateway-summary-card__desc">' . esc_html($description) . '</div>'; | ||
| $european_countries = ['AT', 'BE', 'BG', 'CH', 'CY', 'CZ', 'DE', 'DK', 'EE', 'ES', 'FI', 'FR', 'GB', 'GR', 'HR', 'HU', 'IE', 'IS', 'IT', 'LI', 'LT', 'LU', 'LV', 'MT', 'NL', 'NO', 'PL', 'PT', 'RO', 'SE', 'SI', 'SK']; |
There was a problem hiding this comment.
this array and the rules around it exist in three places:
- here
GeneralSettings.php:294GeneralSettings.php:683
all three have the same 32 country codes, the same "5 or more and all european means Europe" rule, and the same "6 or more currencies means Multi-currency" rule. add a country later and someone has to remember all three spots. more likely they drift, and then the list page and the settings card show different labels for the same method.
one method on this class that takes the gateway and returns the two labels would serve all three callers.
one more thing while you are in there: Global, Europe and Multi-currency are plain strings, not wrapped in __(). everything else on these pages is translatable and this PR adds new strings to the nl, de and fr files, so these three stay english in every language.
| * | ||
| * @return void | ||
| */ | ||
| public function admin_inline_script_hosted_fields_toggle() |
There was a problem hiding this comment.
this is javascript written inside a php class. the project already has a home for admin js:
library/jsholds the admin scriptsPaymentSetupScripts::handleAdminAssets()enqueues them onadmin_enqueue_scripts
so this should be a file in library/js, enqueued there, with the three field ids passed in through wp_localize_script instead of echoed into the markup. as a bonus the file then gets a version number and can be cached, which inline output cannot.
same for the other two inline blocks this PR adds: AbstractPaymentGateway.php:507 and GeneralSettings.php:625.
| /** | ||
| * Allow hosted_fields_client_id to be empty when redirect mode is selected. | ||
| */ | ||
| public function validate_hosted_fields_client_id_field($key, $value) |
There was a problem hiding this comment.
these two methods do nothing.
both fields are declared as type password (lines 290 and 295). WC_Settings_API::get_field_value() looks for validate_{key}_field first, then falls back to validate_{type}_field. without these overrides it calls validate_password_field. with them it calls validate_password_field. same result.
the docblocks say they allow an empty value in redirect mode, but there is no code here doing that, and the fields were never required to begin with.
drop both.
| } | ||
|
|
||
| $settings = [ | ||
| public function get_general_top_settings() |
There was a problem hiding this comment.
this array path is not used anymore.
get_general_top_settings() is only called by get_general_settings(), and get_general_settings() has no callers left. output() renders section '' by hand now (lines 85 to 92), and save() gets its field list from get_section_field_keys().
so these are dead too:
- the
buckaroo_submeniuentry andrender_submeniu_field() - the
buckaroo_payment_listentry (render_payment_list()is still used, but through the direct call, not the hook) render_api_credentials_card()at line 527, only reachable through thebuckaroo_api_credentialsfield type, and no field uses that type- the matching
add_actioncalls in the constructor
if the general section is hand rendered now, delete the array version. keeping both means two places describe the same page, and they will not stay in sync.
2 participants
No description provided.