Adds basic settings screen #84
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams Looks great! A few minor points of feedback.
| public function register_settings(): void { | ||
| // Default implementation does nothing. | ||
| // Child classes can override to register custom settings. | ||
| } | ||
|
|
||
| /** | ||
| * Renders experiment-specific settings fields. | ||
| * | ||
| * Override this method in child classes to render custom settings UI | ||
| * that will appear within the experiment's card on the settings page. | ||
| * This is called after the experiment's main toggle control. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return void | ||
| */ | ||
| public function render_settings_fields(): void { | ||
| // Default implementation does nothing. | ||
| // Child classes can override to render custom settings UI. | ||
| } |
There was a problem hiding this comment.
Just thinking out loud here, not trying to seek perfection, but this made me think of the idea of Experiments providing a settings schema, via something like settings_schema(), and then having Settings_Page register and render any provided settings, so the Experiments themselves wouldn't have to handle all the registration and display logic. Would provide more consistency in rendering, and the Experiment would only need to provide the data structure.
At any rate, I really like the simplified architecture here.
There was a problem hiding this comment.
Oh for sure! I think something like that is very much worth considering. This PR is erring on the side of speed. I didn't want to make any schemas that we're committing to until we have a better idea of what we want the settings to ultimately become.
|
In closing out #51 in favor of this PR, let's make sure whoever makes the merge commit here that we capture the co-authored by details in the comment above as well as those credited for the effort in #51 (comment): |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams Excellent, LGTM!
What?
Closes #25
This introduces a basic settings screen for discovering Experiments, disabling the whole thing, and enabling individual Experiments. It's simple by design, as this is just meant to be a basic starting point as we build things out.
Experiment Settings
It's possible for an experiment to render its own content within its settings card. The
Abstract_Experimentclass has two methods that can be overloaded:register_settings()andrender_settings_fields(). In the former is when to use the WordPress register_setting function, while the latter takes echoed output and places it in the appropriate card, which can include inputs. There's aAbstract_Experiment::get_field_option_name()method for registering settings and rendering inputs, as well.Why?
Users will need a way of toggling which Experiments they want to use, and configuring Experiment settings.
How?
This solves this as simply as possible. Later we can do things like differentiate between user-intended and dev-intended Experiments, make the UI much prettier with modern elements, and so forth.
Testing Instructions
Screenshots or screencast
Global settings disabled
Enabled with Experiments toggled
Experiment with no settings but extra content
Experiment with unique setting (saved and persisting)
Test using WordPress Playground
The changes in this pull request can be previewed and tested using this WordPress Playground instance:
Click here to test this pull request.