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 debounce mechanism to opening the mention panel #4619

Closed
jodator opened this issue Mar 26, 2019 · 5 comments · Fixed by ckeditor/ckeditor5-mention#65
Closed

Add debounce mechanism to opening the mention panel #4619

jodator opened this issue Mar 26, 2019 · 5 comments · Fixed by ckeditor/ckeditor5-mention#65
Assignees
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@jodator
Copy link
Contributor

jodator commented Mar 26, 2019

We support the asynchronous feeds but when the reading items from the API takes noticable time it may happen that user will type faster then the mention UI is showing resulting in showing updating the list in the the UI multiple times.

@Mgsy Mgsy changed the title Add debounce mechanism to opening the mention panel. Add debounce mechanism to opening the mention panel Apr 2, 2019
@oleq
Copy link
Member

oleq commented Apr 3, 2019

Kapture 2019-04-03 at 16 05 58

Feed function

function getFeed( feedText ) {
	return new Promise( resolve => {
        setTimeout( () => {
			const itemsToDisplay = [
				{ id: '1', name: 'Barney Stinson', username: 'swarley' },
				{ id: '2', name: 'Lily Aldrin', username: 'lilypad' },
				{ id: '3', name: 'Marshall Eriksen', username: 'marshmallow' },
				{ id: '4', name: 'Robin Scherbatsky', username: 'rsparkles' },
				{ id: '5', name: 'Ted Mosby', username: 'tdog' }
			].filter( item => {
				const searchString = feedText.toLowerCase();

				return item.name.toLowerCase().includes( searchString ) || item.username.toLowerCase().includes( searchString );
			} )

            resolve( itemsToDisplay );
        }, 5000 );
    } );
}

@oleq
Copy link
Member

oleq commented Apr 3, 2019

This will also produce errors if, for instance, followed by Esc:

Kapture 2019-04-03 at 16 08 20

@oleq
Copy link
Member

oleq commented Apr 3, 2019

A feed with a random response time and no order guarantee is even more red:

function getFeed( feedText ) {
    return new Promise( resolve => {
        const timeout = Math.floor( Math.random() * ( 3000 - 1000 ) + 1000 );

        console.log( 'starting timeout', timeout, 'ms' );

        setTimeout( () => {
            const itemsToDisplay = [
                { id: '1', name: 'Barney Stinson', username: 'swarley' },
                { id: '2', name: 'Lily Aldrin', username: 'lilypad' },
                { id: '3', name: 'Marshall Eriksen', username: 'marshmallow' },
                { id: '4', name: 'Robin Scherbatsky', username: 'rsparkles' },
                { id: '5', name: 'Ted Mosby', username: 'tdog' }
            ].filter( item => {
                const searchString = feedText.toLowerCase();

                return item.name.toLowerCase().includes( searchString ) || item.username.toLowerCase().includes( searchString );
            } )

            console.log( 'ending timeout', timeout );
            resolve( itemsToDisplay );
        }, timeout );
    } );
}

@Reinmar
Copy link
Member

Reinmar commented Apr 8, 2019

We should test this with real implementations of client-server communication. We could build a super simpler server in Node.js which would simulate querying some DBs (setTimeout with a random delay should do). Based on that we should develop a reference implementation of a mention's feed function which communicates with that server and check what we need to change in the mention plugin implementation to make it relatively easy to handle potential issues (stacking requests, wrong response order, etc.)

@mlewand
Copy link
Contributor

mlewand commented Apr 26, 2019

I checked the t/4 branch and here are the findings so far:

  • The delays should be increased. It would be nice to have some responses delayed as much as couple of seconds. Rather than working on a range of random values, I'd use a set of timeouts that a human can tell the difference, e.g. 150, 400, 1000, 2000 nad 4000 ms. So it could be distributed: 60 % for 150 ms response, 40% for any of the remaining values.
  • To keep things clean we should move the dev-server dir away from the root. Do we have already some common place to put such things? I'm thinking about nesting it in ckeditor5-mentions/tests/_utils/asyncserver.

The cache control box is fine.

If it could be easily developed I'd actually ping/fetch a dummy response from dev-server to make sure it is up and running. If not display a clear big information that the server is required and how to do that (basically the information you included on a left hand side). If you're concerned about hitting worst response time, you could allow for a get parameter like http://localhost:3000/?search=chr&noDelay=1 so that the server does not add fake delay.

From other unrelated things: only now I see flickering in mentions sample, so only here I'm able to reproduce ckeditor/ckeditor5-mention#29.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-mention Oct 9, 2019
@mlewand mlewand added this to the iteration 27 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature. package:mention labels Oct 9, 2019
@mlewand mlewand modified the milestones: iteration 27, iteration 28 Oct 16, 2019
@Reinmar Reinmar modified the milestones: iteration 28, iteration 29 Nov 25, 2019
mlewand added a commit to ckeditor/ckeditor5-mention that referenced this issue Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants