Framework and admin screen to add new features as opt-in #113
Conversation
cbravobernal
changed the title
cbravobernal
force-pushed
the
add/new-experiments-menu
branch
from
75511d7 to
b4a9587
Compare
assets/src/js/README.md
Outdated
|
|
||
| To add a new experiment: | ||
|
|
||
| 1. Create a new experiment class in `includes/admin/experiments/` |
There was a problem hiding this comment.
Is this a "good practices" recommendation or a requirement? What's the reason to create the experiments in this folder instead of creating them in the place they belong, and just registering?
There was a problem hiding this comment.
Ideally, this way it should be easier to find all experiments registered instead of having to dig into the entire codebase. Also, I'm trying to follow the PHP class based plugin structure, which may differ from how we did things in Gutenberg.
Is more a recommendation than a requirement, but this way all the admin interface will be automatically generated on PHP.
There was a problem hiding this comment.
Let's remove the readme to prevent external devs to create more experiments.
|
It would be helpful to extract all changes related to code formatting to another PR and land separately, as it makes the review process more complex. What is the reason GitHub reports so massive changes in the lock file? |
Sure, done in #115
It seems it was due to the update of wp-scripts and all its dependencies. I will take a look at it. |
cbravobernal
force-pushed
the
add/new-experiments-menu
branch
2 times, most recently
from
2acfc24 to
609f95d
Compare
This reverts commit 2ec1925.
cbravobernal
force-pushed
the
add/new-experiments-menu
branch
from
609f95d to
8636385
Compare
cbravobernal
added
[Type] Enhancement
| <p><?php esc_html_e( 'No beta features are currently available.', 'secure-custom-fields' ); ?></p> | ||
| </div> | ||
| <?php else : ?> | ||
| <form method="post" action=""> |
There was a problem hiding this comment.
We can be more explicit, leaving it empty was using the current admin url page.
assets/src/js/experiments/types.ts
Outdated
| declare global { | ||
| interface Window { | ||
| acf: ACF; | ||
| acfExperiments?: ExperimentsData; // Variable created by wp_localize_script in admin-experiments.php L150. |
There was a problem hiding this comment.
Nit: I don't think exact line reference will age well. It could be also the case with the file name, so if you want to provide more context, you can list the PR instead.
priethor
changed the title
|
Made a huge refactor:
|
includes/admin/beta-features/class-scf-beta-feature-editor-sidebar.php
Outdated
Show resolved
Hide resolved
I made everything as private as possible. But the approach of each experiment having its custom class, extending the main one, needs to be public 🤔 Maybe we can move that approach to a different one, where every experiment is inside that class. That way we could avoid external devs from creating experiments. Less modular, but less extensible too. |
There was a problem hiding this comment.
But the approach of each experiment having its custom class, extending the main one, needs to be public 🤔
Yep, and the same goes with the hooks. I was just surprised by the amount of "Since version X" I found 😅
This is good to merge and start using, in my opinion. While these beta features are not meant for extenders of the plugin, I still think it would be good to have a short readme to explain SCF contributors how to add new beta features. Not a blocker, though!
I prefer to start with a couple of experiments to test and iterate and then, when we are 100% sure, add the readme. 😅 Thee easiest and fastest way to check for an experiment to exist is just to check the option, so we may save some functions. |
This PRs sets a standard for creating experiments and checking if they are enabled in JavaScript.
As there are no experiments ready yet, I commented all the code that initializes them with a
// Temporarily disabledcomment.The PR was getting big, so it is better to land this first and work later on next experiments.
If you want to test this out, just uncomment the lines // Temporarily disabled in
admin-experiments.phpfile and you will have something like this: