Skip to content

PR for Jason #1

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.

Sign up for GitHub

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

Open
mxstbr wants to merge 1 commit into
base: base-for-jason
Choose a base branch
from
Open

PR for Jason #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 131 additions & 0 deletions index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,135 @@
Author URI: https://mxstbr.com
License: MIT
*/

// 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');
Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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( ... );
}

Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

wp_register_script(
'feedback-fish',
"https://feedback.fish/ff.js?pid=$projectId",
[],
null,
true
);
wp_enqueue_script('feedback-fish');
}
add_action('wp_enqueue_scripts', 'enqueue_feedback_fish_script');

// Add defer="defer" to the feedback.fish <script> tag to avoid blocking render
// Taken from: https://wordpress.stackexchange.com/a/38335
add_filter(
'script_loader_tag',
function ($tag, $handle) {
if ('feedback-fish' !== $handle) {
return $tag;
}

return str_replace(' src', ' defer="defer" src', $tag);
},
10,
2
);

// Add "Send feedback" to the primary nav if it exists
function add_feedback_fish_nav_menu_item($items, $args)
{
$current_user = wp_get_current_user();
$manual_usage = get_option('feedback_fish_manual');
Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

if (!$manual_usage) {
$items .= "<li class=\"menu-item\"><a href=\"#\" data-feedback-fish-userid=\"$current_user->user_email\" data-feedback-fish>Send feedback</a></li>";
}
return $items;
}
add_filter('wp_nav_menu_items', 'add_feedback_fish_nav_menu_item', 10, 2);

// Add "Feedback Fish" settings page to the WPAdmin menu
function create_feedback_fish_settings_page()
{
$page_title = 'Feedback Fish Settings';
Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$menu_title = 'Feedback Fish';
$capability = 'manage_options';
$slug = 'feedback_fish';
$callback = 'feedback_fish_settings_page_content';

add_submenu_page(
'plugins.php',
$page_title,
$menu_title,
$capability,
$slug,
$callback
);
}
add_action('admin_menu', 'create_feedback_fish_settings_page');

// General content for the settings page
function feedback_fish_settings_page_content()
{
?>
<div class="wrap">
<h2>Feedback Fish</h2>
<form method="post" action="options.php">
<?php
settings_fields('feedback_fish');
do_settings_sections('feedback_fish');
submit_button();?>
</form>
</div>
<?php
}

// Setup the settings page sections
add_action('admin_init', 'setup_feedback_fish_settings_page_sections');
add_action('admin_init', 'setup_feeback_fish_settings_fields');
function setup_feedback_fish_settings_page_sections()
{
add_settings_section(
'feedback_fish_general',
'General',
'feedback_fish_settings_page_section_callback',
'feedback_fish'
);
}

function setup_feeback_fish_settings_fields()
{
add_settings_field(
'feedback_fish_project_id',
'Project ID',
'feedback_fish_project_id_field',
'feedback_fish',
'feedback_fish_general'
);
add_settings_field(
'feedback_fish_manual',
'Manual Usage (advanced)',
'feedback_fish_manual_field',
'feedback_fish',
'feedback_fish_general'
);
register_setting('feedback_fish', 'feedback_fish_project_id');
Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to validation, you also want the sanitize_callback to ensure users are putting safe content in the database.

See: https://codex.wordpress.org/Validating_Sanitizing_and_Escaping_User_Data#Sanitizing:_Cleaning_User_Input

register_setting('feedback_fish', 'feedback_fish_manual');
}

function feedback_fish_manual_field()
{
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>";
Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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="' .
Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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>';
Copy link
Copy Markdown

@jasonbahl jasonbahl Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_settings_page_section_callback()
{
}
?>