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

Add/2793 hashlink autocompletion #3324

Closed
wants to merge 11 commits into from

Conversation

BoardJames
Copy link

Description

This implements link autocompletion with '#' as the trigger. It can link to posts, pages, tags or categories. If possible I'd like to also suggest anchors within the document which might be possible with an appropriate querySelectorAll call (suggestions on how that would work are welcome).

At the moment I'd like feedback on if this is the right sort of information to display in the autocomplete.

How Has This Been Tested?

I've tried it out in a few browsers though I should probably do a more rigorous test before merge.

Screenshots (jpeg or gifs if applicable):

selection_057

Types of changes

New feature.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@BoardJames BoardJames added the [Status] In Progress Tracking issues with work in progress label Nov 3, 2017
@BoardJames BoardJames self-assigned this Nov 3, 2017
@BoardJames BoardJames requested a review from jasmussen November 3, 2017 06:58
@codecov
Copy link

codecov bot commented Nov 3, 2017

Codecov Report

Merging #3324 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3324      +/-   ##
==========================================
- Coverage   35.29%   35.21%   -0.08%     
==========================================
  Files         267      267              
  Lines        6744     6758      +14     
  Branches     1221     1221              
==========================================
  Hits         2380     2380              
- Misses       3687     3701      +14     
  Partials      677      677
Impacted Files Coverage Δ
blocks/library/paragraph/index.js 29.16% <ø> (ø) ⬆️
blocks/autocompleters/index.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cc7b8c...ebe89a4. Read the comment docs.

@@ -125,3 +132,56 @@ export function userAutocompleter() {
onSelect,
};
}

export function hashtagAutocompleter() {
const c = wp.api.collections;
Copy link
Member

Choose a reason for hiding this comment

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

Destructuring can help here to avoid using abbreviated c const:

const { Categories, Pages, Posts, Tags } = wp.api.collections;

Copy link
Author

Choose a reason for hiding this comment

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

Good idea

@jasmussen
Copy link
Contributor

I love this! Very very cool.

Could work here be expanded to username autocompletion with @? Doesn't have to be in this PR, but it does get me excited.

This should probably be limited to searching tags, though, less so categories and pages. Those are perhaps best linked using the search tool in the link box.

I also got multi select to invoke by not doing a plain click but a click drag in the menu:

multiselect

There's also been talk about "syntactic sugar" — that is, using markdown shortcuts for some blocks. In this case we'd want this search tool to only invoke on #, and leave # (hash mark followed by a space) for headings.

@jasmussen
Copy link
Contributor

jasmussen commented Nov 3, 2017

I'm being told that user auto completion is already in the works, and builds upon this. Which is just great. 🎉🏅 Edit — already works! I just didn't wait that one second for the list to populate.

As you were.

@kevinwhoffman
Copy link
Contributor

If possible I'd like to also suggest anchors within the document which might be possible with an appropriate querySelectorAll call (suggestions on how that would work are welcome).

@EphoxJames The most common use of linking to anchors within the same document would probably be linking to headings. For that to happen, we would have to auto-assign IDs to each heading and have those IDs included on the front-end. Currently headings get an ID like id="mce_1" within Gutenberg, but that ID isn't available on the front-end.

This should probably be limited to searching tags, though, less so categories and pages. Those are perhaps best linked using the search tool in the link box.

@jasmussen I'm not sure what you mean here. Would it really be that common to link to tag archives? To me, the use of # implies an anchor.

@jasmussen
Copy link
Contributor

I'm not sure what you mean here. Would it really be that common to link to tag archives? To me, the use of # implies an anchor.

Hashtag autocompletion is a common pattern on Facebook, Twitter, P2 and many other social platforms. It'd be a nice feature to have for a modularized Gutenberg. Given you can already add HTML anchors to headings in Gutenberg, I'm not sure an autocomplete for reusing anchors makes a ton of sense to prioritize.

@gziolo
Copy link
Member

gziolo commented Nov 6, 2017

You can use git rebase origin/master to pull the latest changes from master branch. This command doesn't create merge commits and allows you to fix conflicts directly on your existing commit.

@gziolo
Copy link
Member

gziolo commented Nov 6, 2017

I tested on a paragraph and it works properly. It triggers autocomplete when typing # outside of anchor. It also works properly and doesn't trigger autocomplete when typing # inside of HTML nested inside of the anchor. It's pretty slick 👍

Personally, I'm not convinced if displaying autocomplete instantly when you type only # is the right choice here as it produces very random hints pulling all existing content. Should we wait until the first character is typed?

The order of items is also very random, it would be nice to have them sorted by name at least within the type:
screen shot 2017-11-06 at 08 33 21.

At the moment filtering of items is performed on the client. I'm wondering how will this work for a website containing over 100 pages of pages, posts, categories and tags? Would it still only fetch the first page of results? I also noticed that a network request is triggered each time you type #. At least short time caching should be considered here to save some bandwidth.

It seems like a general issue with the autocomplete but when I type esc to hide the box it appears again when I type next char. This might be annoying when someone wants to type #55 or something similar inside the paragraph.

@BoardJames
Copy link
Author

I've done some work on making the ESC keep the popover hidden on #3343

@BoardJames
Copy link
Author

BoardJames commented Nov 8, 2017

From reading the feedback I will:

  1. Only display the list once there is the triggerPrefix + one character
  2. Sort the groups ( posts, pages, tags or categories) alphabetically.

Currently I am not aware of any API to filter the lists before sending them from the server so lots of content is a valid concern but I don't know how to solve it.

The API request on new '#' was a deliberate design decision on the autocompleter - I originally only requested the data once per page load per autocompleter but review highlighted this as incorrect so I changed it to request on opening an autocompleter.
Note I think that the caching (if we implement it) should be done on the API side where everything on the page benefits from it.

@BoardJames
Copy link
Author

Ok, I just tried displaying the list only after one character and it feels unresponsive. I then when looking for existing implementations and both facebook and slack display the menu straight away. So I am not going to implement that.

@BoardJames
Copy link
Author

I have sorted the entries within each type alphabetically.

@BoardJames BoardJames removed the [Status] In Progress Tracking issues with work in progress label Nov 14, 2017
@BoardJames
Copy link
Author

I should note that the reason this hasn't had progress recently is that I am trying to find a way of writting automated tests for it. I initially tried writing unit tests mocking out the collections api but I wasn't happy with how much I was having to mock so I stopped that and tried writing a end-to-end test. Unfortunately it seems that Cypress is unable to simulate selection properly so I could not get a autocompleter popup to display (I should note that the existing Cypress test of the block autocompleter is not actually working despite passing). I then tried putting together a selenium test and it worked very well but I still haven't figured out an elegent way to integrate that into the project.

So what do people think? This has been manually tested so should I merge it, or wait until I can find a way of automating the test?

@youknowriad
Copy link
Contributor

Any update on this one, could we have the feature without the tests for now?

@youknowriad youknowriad added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 2, 2018
@BoardJames
Copy link
Author

Feel free to take over the PR if you wish.

@brandonpayton
Copy link
Member

If we land #4609, it would be easier to test since data retrieval would be separated from rendering. We might not test data retrieval, but we could test rendering, generation of keywords, etc. If that sounds reasonable, I'm up for taking this as a follow up.

@gziolo
Copy link
Member

gziolo commented Jun 10, 2018

There was no activity in this PR for over 6 months. It looks like it was abandoned. Let’s close it. We can always reopen and rebase it with master branch to align with the current state of Gutenberg.

@gziolo gziolo closed this Jun 10, 2018
@gziolo gziolo deleted the add/2793-hashlink-autocompletion branch June 10, 2018 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants