PR for Jason #1
PR for Jason #1
Conversation
| // Inject the feedback.fish JavaScript file into the DOM just before the body | ||
| function enqueue_feedback_fish_script() | ||
| { | ||
| $projectId = get_option('feedback_fish_project_id'); |
There was a problem hiding this comment.
When the plugin is first installed there won't be a value for this option, so this will return false and you will try to add that as a value to the ff.js query param.
I assume that a valid project ID is necessary for the ff.js script to work, so you probably don't want to register/enqueue the script if the settings haven't been filled out yet?
I'd probably suggest something like:
// The 2nd arg "0" is the default value you want in response if there isn't anything in the database yet
$projectId = get_option( 'feedback_fish_project_id', 0 );
// If the id is empty (0), don't register the script
if ( ! empty( $projectId ) ) {
wp_register_script( ... );
}There was a problem hiding this comment.
Also, what happens if an invalid ID is passed in as the pid?
Like if I had a valid project ID input in my settings, but the project was deleted from the Feedback Fish service.
Is ff.js?pid=$deleted_project_id going to cause me issues? Or will it degrade gracefully?
| function add_feedback_fish_nav_menu_item($items, $args) | ||
| { | ||
| $current_user = wp_get_current_user(); | ||
| $manual_usage = get_option('feedback_fish_manual'); |
There was a problem hiding this comment.
Since users can have many menus used in many different places, I'd suggest having it be opt-in to specific menu(s) instead of automatically on for all menus and opt-out.
On the settings page, instead of an option to "I will add the HTML attribute myself" checkbox, you could provide a dropdown of existing menus, and let the user select which Menu to assign feedback fish to.
Then instead of if ( ! $manual_usage ) {:
You could do something like:
// Get the menu that was assigned
$ff_assigned_menu_id = get_option( 'feedback_fish_assigned_menu', 0 );
// If the assigned menu id matches the menu being filtered
if ( ! empty( $ff_menu_id ) && $args->menu === $ff_assigned_menu_id ) {
// do your thing
}| // Add "Feedback Fish" settings page to the WPAdmin menu | ||
| function create_feedback_fish_settings_page() | ||
| { | ||
| $page_title = 'Feedback Fish Settings'; |
There was a problem hiding this comment.
For strings that are rendered in admin UIs, I'd recommend getting in the habit of passing them through translation functions so that in the future you could provide translations for the strings in your plugin and make it useful for many languages.
see: https://codex.wordpress.org/I18n_for_WordPress_Developers#Strings_for_Translation
So, in your case it would become: $menu_title = __( 'Feedback Fish', 'feedback-fish' );
The 2nd argument is your plugin's "text domain" which you define in your plugin docblock like so: https://codex.wordpress.org/I18n_for_WordPress_Developers#Text_Domains
| 'feedback_fish', | ||
| 'feedback_fish_general' | ||
| ); | ||
| register_setting('feedback_fish', 'feedback_fish_project_id'); |
There was a problem hiding this comment.
I imagine there's a way to validate that the ID is for an existing Feedback Fish project? I'd recommend passing a sanitize_callback function where you can validate that the user added a valid project id, and if they did not you could return a validation message.
Like if I enter: fake project as the project ID, you probably don't want to enqueue that to the users front end if you could instead tell the user when they save the setting that hey pal, this project ID is invalid
So, in your sanitize_callback you could, if it's possible, make a remote HTTP request to your service to validate if the project ID exists or not and if not, tell the user on the settings page before the option is saved.
There was a problem hiding this comment.
In addition to validation, you also want the sanitize_callback to ensure users are putting safe content in the database.
| echo '<label><input name="feedback_fish_manual" id="feedback_fish_manual" type="checkbox" value="1"' . | ||
| checked(1, get_option('feedback_fish_manual'), false) . | ||
| ' /> I will add the <code>data-feedback-fish</code> HTML attribute myself.</label>'; | ||
| echo "<p class=\"description\">Enabling this stops the plugin from adding a \"Send feedback\" button to your primary navigation. See the <a href=\"https://feedback.fish/help/widget/\" target=\"_blank\">documentation</a> for more information on the HTML attribute.</p>"; |
There was a problem hiding this comment.
You could do this later, but using the translation functions on your strings will make it easier to internationalize your plugin later.
| echo '<input name="feedback_fish_project_id" id="feedback_fish_project_id" type="text" value="' . | ||
| get_option('feedback_fish_project_id') . | ||
| '" />'; | ||
| echo '<p class="description">You can get your project ID from <a href=\"https://feedback.fish/app\" target=\"_blank\">your Feedback Fish dashboard</a>.</p>'; |
There was a problem hiding this comment.
You could do this later, but using the translation functions on your strings will make it easier to internationalize your plugin later.
|
|
||
| function feedback_fish_project_id_field() | ||
| { | ||
| echo '<input name="feedback_fish_project_id" id="feedback_fish_project_id" type="text" value="' . |
There was a problem hiding this comment.
You'll want to escape the option before outputting it in the text field:
see: https://codex.wordpress.org/Validating_Sanitizing_and_Escaping_User_Data#Escaping:_Securing_Output
No description provided.