-
Notifications
You must be signed in to change notification settings - Fork 12
Admin Settings Screen Implementation #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Admin Settings Screen Implementation #51
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @[email protected]. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #51 +/- ##
============================================
+ Coverage 48.48% 54.40% +5.91%
- Complexity 45 134 +89
============================================
Files 6 16 +10
Lines 198 579 +381
============================================
+ Hits 96 315 +219
- Misses 102 264 +162
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5d3ada5 to
72b4e56
Compare
|
@Ref34t is this PR ready for review or is it still in-progress? |
|
@jeffpaul it's ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the changes here, it seems to avoid permission issues we're adding a comment instead of updating the PR description. I have a separate PR (#65) that fixes permission issues. I'd suggest we remove these changes from this PR and can continue discussion on that other PR to keep things more focused here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note I've brought the code changes here that ensure we have a PR description over to #65, though I didn't bring over the changes that add a comment instead of updating the PR description
| # Build | ||
| /build/ | ||
| /dist/ | ||
| /build/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this change just swaps the position of these two, any reason for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a commit that removed the build directory and then in a later commit it was added it back, just in a slightly different location
| "format": "phpcbf --standard=phpcs.xml.dist", | ||
| "lint": "phpcs --standard=phpcs.xml.dist", | ||
| "phpstan": "phpstan analyse --memory-limit=1G", | ||
| "stan": "phpstan analyse --memory-limit=1G", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for this naming change?
| __( 'AI Experiments', 'ai' ), | ||
| __( 'AI Experiments', 'ai' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth setting a constant for this instead of repeating it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we almost certainly will have other JS/TS files to support each individual feature, wondering if all this code should be nested under src/admin or src/settings or similar just to make it clear as we start introducing other code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In testing this out, I think we're likely to have some iterations on the actual design of the settings page though overall it looks good to me. My one note that I'd suggest we fix in this PR is the first time I enable/disable something, a notice is shown. If I don't close that message out and then enable/disable again, the page content jumps around. I think the issue here is the notice is removed then added back so quickly and causes things to shift up and down.
| // Always register settings sections so the feature appears in admin. | ||
| add_action( | ||
| 'ai_register_settings_sections', | ||
| array( $this, 'register_settings_sections' ) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this a bit more, wondering if it would be better to have this handled within the Abstract_Feature class instead of requiring each individual feature class to have this exact same code?
So basically the abstract class would run this and then each extending class would have to implement their own register_settings_sections method?
| // Only register functional hooks if the feature is enabled. | ||
| if ( ! $this->is_enabled() ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above, right now each feature class would need to have these exact same lines of code. Instead of that duplication, wondering if we introduce a new method in the parent class that is only called if is_enabled returns true. So if you need to register functionality that is always there, even if the feature is disabled, you could do that in the register method (or we could rename that to something else) and if you want to run functionality only if the feature is enabled, you could do that in a new method, something like register_when_enabled
|
|
||
| private const STYLE_FILE = 'build/style-index.css'; | ||
|
|
||
| private const STYLE_FILE_PATH = __DIR__ . '/../../build/style-index.css'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In places where we're traversing the path upwards like this, I wonder if we should use WP_AI_DIR instead?
| private const STYLE_FILE_PATH = __DIR__ . '/../../build/style-index.css'; | |
| private const STYLE_FILE_PATH = WP_AI_DIR . '/build/style-index.css'; |
| $enabled = $this->feature_toggles->is_feature_enabled( $this->id ); | ||
| } else { | ||
| $enabled = $this->enabled; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused on the enablement checks. We have:
$this->feature_toggles->is_feature_enabled( $this->id );$this->enabledapply_filters( "ai_feature_{$this->id}_enabled", $enabled )
It's hard to know what the source of truth is. Do have needs for this complexity already? I wonder if the UI could store the enabled/disabled options somewhere and just use those with the existing filter and avoid all the new classes? Admittedly I don't have a full picture of the next steps and/or plans for this.
Summary
Builds a complete admin settings screen with React UI for managing AI experimental features.
Key Components
Architecture
Feature Development Pattern
Features extend Abstract_Feature and follow these rules:
load_feature_metadata()returning id, label, descriptionai_register_settings_sectionshookis_enabled()before registering functional hooksProvides_Settings_Sectiontrait for settings panelsSee Example_Feature for complete reference implementation.
Files Added
Resolves #25