Editor: Enable linking to existing content

[aduth]

Closes #303
Blocked by #3193, #3575
Cherry-picks dcc54689123a09985fb9cf9f208ff3ac6fd95521 from #2472

This pull request seeks to enable a user to add a link by selecting existing content from their site to which the link should target.

Testing instructions:

Note that there are design flaws that cause the post selector to occupy more space than desirable, likely preventing you from confirming the link selection. These will be remedied with the merge of #3193.

1. Navigate to the Calypso post editor
2. Select a site, if prompted
3. (Optional) Enter text in the post content area and highlight it
4. Click Add / Insert Link in the editor toolbar
5. Note that a selector field is made available to choose existing site content
6. Click an option from the post selector
7. Note that the URL field is populated with the URL of the selected content
8. Note that if you are not adding a link to existing text, i.e. you skipped Step 3, that the title of the content is used as the Text field
9. Note that the radio option is selected for the chosen content

[timmyc]

🍗

[aduth]

🍖 & 🐔

[timmyc]

Very excited to see this becoming a reality - and the PostSelector has been used again :)

A few items:

• I think we should label this with a design review because it does feel a little rough as-is right now
• When I have a long-list of content to choose from, the dialog is quite vertically long once the content is loaded ( which might be a bad experience on mobile ), and while searching, the modal collapses and expands. Perhaps having a fixed height container around the selector, like we have in the page editor sidebar would be a quick fix
• Did you consider having the post selector conditionally shown like it is in wp-admin? So users have to toggle the selector open ( could save some network requests if they don't plan on linking to existing content ).

Some of the experience above can be seen in this .gif - where I also encountered a bug it seems:

Steps to reproduce:

• Type in some content
• Highlight Content, click link button in toolbar
• Use the search box to experience the growing/shrinking modal
• I select some content from the list, radio selects 👍
• Then click cancel
• Any subsequent attempts to open the link dialog do not work. The dialog is not shown again but no errors are logged in the console.

[aduth]

Thanks for the review, @timmyc !

Now that #3193 has been merged, the appearance is now much approved, and I've added a screenshot to the original comment.

I've added the design label in case there's feedback about the current appearance.

When I have a long-list of content to choose from, the dialog is quite vertically long once the content is loaded

I've set the height as fixed at 24vh. Admittedly, I've not tested this on mobile yet, which I'll plan to do in the morning.

Did you consider having the post selector conditionally shown like it is in wp-admin?

It's an option, but I quite like the idea of eliminating this extra step. The additional network request is pretty negligible on our end. To be clear, the request is not made until the dialog is opened.

where I also encountered a bug

I ran into a few bugs myself 😄 The one you noted was due to the fact that the dialog was quite "impure", and was not being properly re-rendered due to react-redux defaulting to pure render for any component wrapped using connect. It's not perfectly pure now, though there should be no issues with opening or closing the dialog as of 849ec23.

The behavior of assigning the link text was also pretty unpredictable. As of ff4eb60, the link text will only be assigned if existing text cannot be determined, either if editing an existing link, or if the user has set a custom link text.

Tomorrow, I'll plan to add some improvements to the <PostSelector /> component in a separate pull request to include a label describing the type of content if type="any" to improve clarity of content types and for consistency with wp-admin.

[timmyc]

Thanks for the notes - I'll do some more testing and do so on mobile and the :transformer: too :)

Since you brought up type="any" - when we have support for CPT's how would that operate? Is any just "post,page"?

[timmyc]

Looking good on the simulator, and with the fixed heigh it is much better. Moving the visibility out of the state and into a prop has fixed the other issue I was seeing too - nice job.

I'll give it a spin on Edge/IE11 as well but this is looking great to me.

One other minor change that might be nice is to add some GA Events / and tracks to these actions. Tracking how often links are added, and if they are linking to existing content would be great data to have.

[aduth]

Since you brought up type="any" - when we have support for CPT's how would that operate? Is any just "post,page"?

Ideally, the REST API simply returns any known posts, regardless of type, if passed type=any. This unfortunately is not the case currently it seems, so only posts and pages will be returned (which is a shame).

Spoke too soon. The REST API already supports custom post types, but Jetpack sites must whitelist any custom post types using the rest_api_allowed_post_types filter:

https://developer.wordpress.com/2013/04/26/custom-post-type-and-metadata-support-in-the-rest-api/

[timmyc]

Spoke too soon. The REST API already supports custom post types, but Jetpack sites must whitelist any custom post types using the rest_api_allowed_post_types

Which will be replaced by show_in_rest in WP-API, but does bring up the question of supporting that filter in WP-API endpoints, or requiring developers to switch over to the WP-API registration syntax.

[aduth]

but does bring up the question of supporting that filter in WP-API endpoints, or requiring developers to switch over to the WP-API registration syntax.

I imagine we could provide some backwards-compatibility for plugin authors who have already whitelisted with the Jetpack filter.

// See: WPCOM_JSON_API_Endpoint
function jetpack_allowed_post_types_compat( $args, $post_type ) {
    if ( in_array( $post_type, $this->_get_whitelisted_post_types() ) ) {
        $args['show_in_rest'] = true;
    }

    return $args;
}
add_filter( 'register_post_type_args', 'jetpack_allowed_post_types_compat', 10, 2 );

[timmyc]

@aduth just tested out on Edge and it looks good. I'm good with this getting merged in, wasn't sure if @hugobaeta wanted to give it a quick design review - but will mark ready to merge.

[aduth]

Thanks @timmyc . I'll hold off a bit in case we can get #3264 and related changes in to improve the initial experience, but will plan to move forward with merging if those turn out to be rabbit holes.

[aduth]

Some last-minute testing revealed a few bugs with <PostSelector /> ordering, especially as subsequent pages were received. @timmyc , would you mind eyeing over the last three commits to see if they make sense to you (f50a44c, 4c4d172, 67b3a9d)? With these changes, the ordering should exactly match that of wp-admin, with the exception of our <PostSelector /> reflecting parent-child relationships in the display of options. It also allows rendering results with search terms as a hierarchy (detached children are shown as top-level).

[timmyc]

@aduth would it make sense to save the sorted state in redux? Even if it were just a sorted set of global_ID pointers? That is the only thing that feels strange after reading over the last 3 commits

[aduth]

@aduth would it make sense to save the sorted state in redux? Even if it were just a sorted set of global_ID pointers?

Relevant to this point: #3522

We tend to encourage using selectors in place of redundant data in the state tree. If we were to introduce something like reselect, it would be reasonable to create a memoized selector for retrieving the treeify'd posts hierarchy. Maybe we should hold off and see where that pull request goes...

[aduth]

There's been some blockers parallel to this pull request which have since been merged, so this is once again ready for final review.

Rebased changes account for:

• Fixing <PostSelector /> order for hierarchical posts, previously committed here, fixed and merged separately at Components: Fix PostSelector order for hierarchical posts #3575
• Removing state.posts.sitePosts reducer from state tree by @gwwar in Framework: remove unused posts.sitePosts from Redux tree #3602. The rebased b91d791 now implements a new memoized getSitePosts selector filtering on site.posts.items.

Also included analytics tracking in 9c4e1b8, as requested by @timmyc.

[gwwar]

👏 Thanks for tidying.

[gwwar]

👍 Code looks good and I tested the linking functionality. Should be good to 🚢 once you get the design +1

[aduth]

once you get the design +1

I'm going to go ahead and remove that label. Iterations on the design of the <PostSelector /> have occurred separately from this pull request, particularly in #3193.