- Issue created by @Leo Pitt
- ๐ฎ๐ณIndia vinayakmk47
Added live mode configuration option with checkbox in admin settings. When disabled, donations will be processed in test mode. Implemented as requested.
- ๐ฌ๐งUnited Kingdom Leo Pitt
@vinayakmk47 thank you for your contribution!
I've reviewed the code, and it looks great overall, but I have a few suggestions and changes to align it with Drupal's coding standards and best practices:
Comment Style
Please ensure all comments end with a full stop. This is a requirement for Drupal's coding standards.
Indentation and Trailing Spaces
Please check the indentation, especially around Line 42 of
/fundraiseup_js.module
. Also, remove any trailing spaces, such as those on Line 43.(running the code through PHP Code Sniffer (
phpcs
) with Drupal standards will help to catch any formatting or style issues like these.)Simplify the
sprintf
UsageI think that using
sprintf
function for the live mode variable is unnecessary. Instead, we can directly interpolate the value, e.g.:'window.fundraiseup_livemode = ' . ($live_mode ? 'true' : 'false') . ';'
We could perhaps cast the
live_mode
value to a boolean when fetching it from configuration. For example:$live_mode = (bool) ($config->get('live_mode') ?? TRUE);
Redundant JS Line?
Thereโs a line in the JavaScript:
window.fundraiseup_livemode = drupalSettings.fundraiseUpJS?.liveMode ?? true;
This seems redundant since weโre already attaching the live mode declaration via a
script
tag. Is it needed?Let me know if you need further clarification or help with these updates. Thanks again for your contribution!
- ๐ฎ๐ณIndia vinayakmk47
@leo pitt I have made changes according to the above mentioned comments Please review it and let me know if there are any further changes i have to make.
- ๐ฌ๐งUnited Kingdom Leo Pitt
Thanks @vinayakmk47. I have made one small update to indentation. Otherwise, this is good.
-
leo pitt โ
committed 6035e44d on 1.0.x authored by
vinayakmk47 โ
Issue #3497600 by vinayakmk47, leo pitt: Allow admins to switch live...
-
leo pitt โ
committed 6035e44d on 1.0.x authored by
vinayakmk47 โ
- ๐ฎ๐ณIndia vinayakmk47
Thanks, @leo pitt, for your feedback and for making the indentation update! I appreciate your review and confirmation that the changes are good. Let me know if there's anything else you'd like me to address.
- ๐ฌ๐งUnited Kingdom Leo Pitt
No problem, great work @vinayakmk47 and thanks for the contribution.