Admin settings refactor - WP_Auth0_Admin_Generic #416
Conversation
Admins settings have confusing wording, inconsistent behavior, and broken links and translation. This first PR refactors the description system to be more straight-forward, adds HTML generation functions for consistent field outputs (used in future commits), adds settings page description translation, and fixes a few other minor code issues. First of a few smaller PRs to replace #396
| @@ -3,7 +3,7 @@ class WP_Auth0_Api_Operations { | |||
|
|
|||
| protected $a0_options; | |||
|
|
|||
| public function __construct( WP_Auth0_Options $a0_options ) { | |||
| public function __construct( WP_Auth0_Options_Generic $a0_options ) { | |||
There was a problem hiding this comment.
is this a breaking change?
There was a problem hiding this comment.
No, WP_Auth0_Options extends WP_Auth0_Options_Generic
| public function __construct( WP_Auth0_Options_Generic $options, WP_Auth0_Routes $router ) { | ||
| parent::__construct( $options ); | ||
| $this->router = $router; | ||
| $this->_description = __( 'Settings related to specific scenarios.', 'wp-auth0' ); |
There was a problem hiding this comment.
should this be localized (i18n)?
There was a problem hiding this comment.
__() is the WP localizing function
| @@ -2,12 +2,25 @@ | |||
|
|
|||
| class WP_Auth0_Admin_Basic extends WP_Auth0_Admin_Generic { | |||
|
|
|||
| // TODO: Deprecate | |||
| const BASIC_DESCRIPTION = 'Basic settings related to Auth0 credentials and basic WordPress integration.'; | |||
There was a problem hiding this comment.
was this public or why can't it be deleted instead of deprecated?
There was a problem hiding this comment.
Constants are public, can't be anything but. Unlikely that anyone would be using it but ...
There was a problem hiding this comment.
What if you change the content of the constant but keep it named like that? and then use it from the description at line 21.
const BASIC_DESCRIPTION = 'Basic settings related to the Auth0 integration.';
//....
$this->_description = __( BASIC_DESCRIPTION, 'wp-auth0' );that way you don't need to deprecate it.
There was a problem hiding this comment.
Might be nice but __() does not accept a variable for a string to translate because of how those are generated
| */ | ||
| public function __construct( WP_Auth0_Options_Generic $options ) { | ||
| parent::__construct( $options ); | ||
| $this->_description = __( 'Basic settings related to the Auth0 integration.', 'wp-auth0' ); |
There was a problem hiding this comment.
should this be localized (i18n)?
| @@ -3,20 +3,29 @@ | |||
| class WP_Auth0_Admin_Generic { | |||
|
|
|||
| protected $options; | |||
| protected $_option_name; | |||
| protected $_description; | |||
| protected $_textarea_rows = 4; | |||
There was a problem hiding this comment.
shouldn't this be _text_area_rows or what is the convention you are following?
There was a problem hiding this comment.
textarea like the HTML element
| * Render description at the top of the settings block | ||
| */ | ||
| public function render_description() { | ||
| if ( ! empty( $this->_description ) ) { |
There was a problem hiding this comment.
this._description is never set. On the contrary, _option_name is set on the __construct method
There was a problem hiding this comment.
Extending classes set this, parent class doesn't need one
There was a problem hiding this comment.
Hmm OK. It's a little weird to me, but if this is they way you do it on php I'm fine.
| * Wrapper for add_settings_error | ||
| * | ||
| * @param string $error - translated error message | ||
| */ | ||
| protected function add_validation_error( $error ) { | ||
| add_settings_error( |
There was a problem hiding this comment.
where is this method? add_settings_error
Admin settings have confusing wording, inconsistent behavior, and
broken links and translation. This first PR refactors the description
system to be more straight-forward, adds HTML generation functions for
consistent field outputs (used in future commits), adds settings page
description translation, and fixes a few other minor code issues.
First of a few smaller PRs to replace #396