-
Notifications
You must be signed in to change notification settings - Fork 156
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
Pull screen search feature #1256
base: develop
Are you sure you want to change the base?
Conversation
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've added some notes inline.
The UI looks good but I'm hitting a bug with the search. Otherwise the notes a minor.
); | ||
const dropdown = container.querySelector( '.searchable-select__dropdown' ); | ||
|
||
const itemss = await fetch( '/wp-admin/admin-ajax.php', { |
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.
Typeo? If it's meant to be searchResults or similar then use the longer name for clarity.
const itemss = await fetch( '/wp-admin/admin-ajax.php', { | |
const items = await fetch( '/wp-admin/admin-ajax.php', { |
|
||
function filterItems( searchTerm ) { | ||
const filteredItems = itemss.filter( ( item ) => | ||
item.toLowerCase().includes( searchTerm.toLowerCase() ) |
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 getting an error on this line as item
is an object:
{
"id": "2",
"name": "ms-distributor.local/two",
"url": "ms-distributor.local/two",
"pull_url": "http://ms-distributor.local/wp-admin/admin.php?page=pull&connection_type=internal&connection_id=2",
"type": "internal"
}
'name' => untrailingslashit( $internal_connection->site->domain . $internal_connection->site->path ), | ||
'url' => untrailingslashit( preg_replace( '#(https?:\/\/|www\.)#i', '', get_site_url( $internal_connection->site->blog_id ) ) ), |
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 little unclear why name and URL are using the same (or at least similar ) values. site->domain . site->path
ought to resolved to get_site_url()
.
Although we don't display the name, it may be helpful to allow people to search by the name, so a search for Make WordPress Core
would return make.wordpress.org/core
add_action( 'wp_ajax_dt_load_connections', __NAMESPACE__ . '\get_connections' ); | ||
add_action( 'wp_ajax_dt_load_connections_push', __NAMESPACE__ . '\get_connections' ); |
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 assuming this is for clarity from wp_ajax_dt_load_connections_pull
?
We're probably stuck with the old name to ensure backward compatibility with the third party extensions and between different versions of the plugin. Otherwise we could go through a soft deprecation.
Description of the Change
Add ability to search for a site on the Pull Content screen
Closes #1235
How to test the Change
Changelog Entry
Credits
Props @kirtangajjar
Checklist: