Issue-97: Remove dependency on Fieldmanager and manually create admin pages #121
Conversation
| .then((response) => { | ||
| setFooterSettings(response as any as FooterSettings); | ||
| setSettings(response as any as Settings); |
There was a problem hiding this comment.
I recognize that you didn't change the underlying code here, but it would be good to validate that what comes back from the API is in the proper shape. What I usually do here is create a function that takes an unknown parameter and guarantees a return type, and throws if it can't. The zod library is very good for this and I can help point you in the right direction for how to implement it (it's pretty straightforward but I have examples I can share).
| } | ||
| } | ||
|
|
||
| export default function ImagePicker({ |
There was a problem hiding this comment.
We have an image picker component available as part of block-editor-tools which I would like to use here if possible: https://github.com/alleyinteractive/alley-scripts/tree/main/packages/block-editor-tools/src/components/image-picker
| // Get all posts with the nb_template post type. | ||
| const [templatePosts, setTemplatePosts] = useState([]); | ||
| useEffect(() => { | ||
| apiFetch({ path: '/wp/v2/nb_template' }) |
There was a problem hiding this comment.
What happens if there are more than $posts_per_page templates? At a minimum we should fetch 100 per page, and it would be good to check the total number of pages and continue to fetch until we've got them all.
| */ | ||
| const { createErrorNotice, createNotice, removeAllNotices } = noticeOperations; | ||
| const saveSettingsData = () => { | ||
| onSave().then(() => { |
There was a problem hiding this comment.
Generally I prefer the async/await syntax vs. promises (it eliminates additional nesting). You can wrap an await call in try/catch to get the same behavior of .catch() and .finally() and so on.
| isSaving = false, | ||
| onChange = () => {}, | ||
| onSave = () => {}, | ||
| } = useOption('nb_settings'); |
There was a problem hiding this comment.
I think we should make a custom hook for working with Newsletter Builder settings that does a few things:
- It would wrap
useOptionso that we would eliminate duplicate code where we're extracting values from the settings object. - We could verify that the value returned from the call is of the proper shape and type so TypeScript is happy (and could convert this file to a TypeScript file while we're at it).
- We could establish defaults when values don't exist.
| * @param {string} key Key to update. | ||
| * @param {string} value Value to update. | ||
| */ | ||
| const updateSettingsData = (key, value) => { |
There was a problem hiding this comment.
By creating our own custom hook we could also encapsulate this functionality so that the implementing component gets a getter and a setter for each value, for instance.
| // add_action( 'init', [ $this, 'maybe_register_settings_page' ] ); | ||
| // add_filter( 'pre_update_option_' . static::SETTINGS_KEY, [ $this, 'sort_settings_on_save' ], 10, 1 ); |
| * Enqueue scripts and styles for the settings page. | ||
| */ | ||
| public function enqueue_assets() { | ||
| wp_enqueue_script( 'wp-newsletter-builder-admin-email-types' ); |
There was a problem hiding this comment.
Should this logic only be fired on the page where settings are set vs. on all admin pages?
| public function maybe_register_settings_page(): void { | ||
| if ( function_exists( 'fm_register_submenu_page' ) && \current_user_can( 'manage_options' ) ) { | ||
| \fm_register_submenu_page( static::SETTINGS_KEY, 'edit.php?post_type=nb_newsletter', __( 'Email Types', 'wp-newsletter-builder' ), __( 'Email Types', 'wp-newsletter-builder' ) ); | ||
| \add_action( 'fm_submenu_' . static::SETTINGS_KEY, [ $this, 'register_fields' ] ); | ||
| \fm_register_submenu_page( static::SETTINGS_KEY, 'edit.php?post_type=nb_newsletter', __( 'Email Types', 'wp-newsletter-builder' ), __( 'Email Types', 'wp-newsletter-builder' ) ); |
There was a problem hiding this comment.
Will need to remove the FM call here since we're fully removing FM as a dependency.
| 'schema' => [ | ||
| 'type' => 'array', | ||
| 'properties' => [ | ||
| 'uuid4' => [ |
There was a problem hiding this comment.
I'd love to figure out how to not require a UUID for the settings order - maybe this is established in JS only and not saved to the DB? Maybe we find another way to establish keys to avoid issues with sorting?
Summary
This PR aims to address the issue outlined in #97, which involves removing the dependency on Fieldmanager for generating admin settings pages within WP Newsletter Builder and manually creating these settings pages instead. This change is intended to simplify the installation process for developers by eliminating the need for an additional plugin. Fixes #97
Changes
Additional Information
During this process, consideration was given to utilizing Gutenberg for the admin edit page, as mentioned in the issue's description and supported by examples and suggestions in the issue's comments. Specifically, the use of the
useOptionhook fromalleyinteractive/alley-scriptswas proposed, along with the potential development of additional hooks or helpers for managing settings pages with Gutenberg.Test Instructions
References
useOptionhook suggestion: useOption Hook