Skip to content
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

Editable: Display a posts list suggestion in the link modal #1985

Merged
merged 4 commits into from
Jul 26, 2017

Conversation

youknowriad
Copy link
Contributor

closes #1040

This is not ready but I'd appreciate some eyes to help me polish it.
The idea is to provide a dropdown of suggested posts when adding a link.

screen shot 2017-07-24 at 13 58 25

Some notes:

  • I'm only looking for posts, the issue suggests that we should look for pages too but unless I'm missing something the REST API can't look for both pages and posts. I might have to trigger two requests.
  • The search is triggered after typing at least 3 characters
  • This needs some Accessibility eyes, We can navigate using arrows but I guess this is far from perfect.

@youknowriad youknowriad added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Jul 24, 2017
@youknowriad youknowriad self-assigned this Jul 24, 2017
@youknowriad youknowriad requested review from jasmussen and aduth July 24, 2017 12:59
@youknowriad youknowriad force-pushed the try/link-posts-suggestions branch from 7b8d01d to 788f300 Compare July 24, 2017 13:01
@aduth
Copy link
Member

aduth commented Jul 24, 2017

unless I'm missing something the REST API can't look for both pages and posts

I think you're correct about this. Maybe worth flagging as "Needs API Endpoint" or otherwise some API conveniences? Two separate requests seems undesirable 😞

@youknowriad youknowriad added the Core REST API Task Task for Core REST API efforts label Jul 24, 2017
@jasmussen
Copy link
Contributor

Nice, thank you for working on this!

At a glance, this appears to be a nice UI that I'd thumb up. I can't really get it to work, though:

jul-24-2017 16-02-18

@youknowriad
Copy link
Contributor Author

@jasmussen I suspect the same problems you encounter when performing API calls. Any JS error?

@jasmussen
Copy link
Contributor

No JS errors, no. First time I tried, a little dialog did pop out though, and the spinner stopped. But nothing populated the dialog.

@youknowriad youknowriad changed the title [WIP] Editable: Display a posts list suggestion in the link modal Editable: Display a posts list suggestion in the link modal Jul 25, 2017
@youknowriad
Copy link
Contributor Author

@nylen maybe you have some thoughts on the API issue here.

@jasmussen Unfortunately, I couldn't reproduce :(

@mtias
Copy link
Member

mtias commented Jul 25, 2017

@youknowriad works great for me on text nodes, but breaks the button on images:

image

@youknowriad
Copy link
Contributor Author

@mtias Good catch, it should be better now (same for the button block)

@mtias
Copy link
Member

mtias commented Jul 25, 2017

@youknowriad cool, but it should open over the whole toolbar area :)

Master:
image

@youknowriad youknowriad force-pushed the try/link-posts-suggestions branch from 1eadc5d to d749721 Compare July 25, 2017 11:17
@youknowriad
Copy link
Contributor Author

I force-pushed the last commit. I was missing a className change.

@@ -0,0 +1,187 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

It is a bit weird that link-input is under format-toolbar, considering the link input can be used inline or block level, what about placing it together with url-input? The current url-input/index.js could be renamed button.js maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I like this proposal, it's there because it was extracted from the format-toolbar at first.

this.suggestionsRequest.abort();
}

if ( value.length < 2 ) {
Copy link
Member

Choose a reason for hiding this comment

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

A comment could help here.

@mtias
Copy link
Member

mtias commented Jul 25, 2017

Just a comment on the component structure, but this looks good to me.

@youknowriad youknowriad force-pushed the try/link-posts-suggestions branch from f1199bf to b0422cf Compare July 25, 2017 11:54
Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

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

This looks good to me. 🚢

@youknowriad youknowriad merged commit b749af4 into master Jul 26, 2017
@youknowriad youknowriad deleted the try/link-posts-suggestions branch July 26, 2017 08:42
@nylen
Copy link
Member

nylen commented Jul 26, 2017

The API issue here is tracked at https://core.trac.wordpress.org/ticket/39965.

@afercia
Copy link
Contributor

afercia commented Jul 29, 2017

I'm only looking for posts, the issue suggests that we should look for pages too but unless I'm missing something the REST API can't look for both pages and posts. I might have to trigger two requests.

The current functionality in WP searches also pages and also CPTs. See
wp_ajax_wp_link_ajax()
_WP_Editors::wp_link_query()

Worth also considering the REST API returns by default all the post fields including the content, and the response could be huge in size. Even several hundreds KB or more than 1MB with very long posts. See related considerations on:
https://core.trac.wordpress.org/ticket/38920
https://core.trac.wordpress.org/ticket/38922
and especially:
"REST API: Provide interface to include or exclude specific fields from response JSON"
https://core.trac.wordpress.org/ticket/38131

I think these concerns should definitely go in a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core REST API Task Task for Core REST API efforts [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links: Search your old posts and pages in the input field
6 participants