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

feat(pat-autosuggest): load more #1195

Merged
merged 1 commit into from
Dec 18, 2024
Merged

feat(pat-autosuggest): load more #1195

merged 1 commit into from
Dec 18, 2024

Conversation

thet
Copy link
Member

@thet thet commented Jan 3, 2024

We can tell select2 to batch or not to batch.
If we batch, we can tell select2 how many items it should load per batch.

Please say how you would like to state these options in terms of pat-autosuggest attributes.

The text below is background information by Hannes.

pat-autosuggest currently does not have ajax batching support aka infinite scrolling.
It can only load a fixed number of items and not load more when the user scrolls down the list.

There is some batching support already prepared, but in it's current form this does not work.

This draft PR adds real batching support for ajax requests.
For the batching support to work, the backend also needs to support it, but more on that later.

We added two options to pat-autosuggest:

ajax-batch-size (default: 10) - This defines the number of items which should be loaded from the backend within one request. The default is 10 - this is the same number we currently have hard-coded in this Pattern.

ajax-batch-mode (default: fixed, possible other value: batched) - This defines if the batching/inifinite scrolling should be enabled or not. The default is fixed, which reflects the current behavior. When set to batched it will request more items if the user reaches the end of the selection list.

The logic to determine if more results should be loaded from the server is simple: If the number of items in the result is lower than the batch size no more results will be loaded. If the number is equal to the batch size, the next batch from the server will be requested. If there are no more items an empty list would be returned.

The server needs to support these request parameters for batching support (these are already submitted to the server):

  • page (Integer): The page number of the batch. 1 for the first result set, 2 for the second and for forth.
  • page_limit (Integer): The batch size. This is the same as the ajax-batch-size parameter.

The server needs to respond with an JSON array of objects with id and text as keys (This is already the case).
If there are no more results, the server needs to return an empty list.

@cornae, please give us feedback on adding this feature in general and the new options and possible naming suggestions in particular.

Ref: syslabcom/scrum#1638

For more information on this see:
https://select2.github.io/select2/#infinite

@thet thet marked this pull request as draft January 3, 2024 15:46
@thet thet requested a review from cornae January 3, 2024 18:05
@thet thet force-pushed the scrum-1638-batching branch from 8aa1a62 to 67c0530 Compare January 3, 2024 18:11
@thet thet requested a review from reinhardt January 4, 2024 09:10
@thet thet removed the request for review from reinhardt January 4, 2024 09:11
@thet thet force-pushed the scrum-1638-batching branch from 67c0530 to 6960bf9 Compare January 4, 2024 09:35
@thet
Copy link
Member Author

thet commented Jan 4, 2024

@reinhardt adding your suggestion from the chat in here:

how about on and off as values for something like result-batching? Is that better than true and false?
otherwise we could call it ajax-result-grouping with values single (or full) and batched/batches

@pilz
Copy link
Member

pilz commented Jul 2, 2024

I noted on 11.June that @cornae wanted to contact @thet on the specifics of this PR to determine how to name the pattern attributes. Could you do this this week? The timeouts we get are causing repeated helpdesk requests from osha and are getting quite disruptive, so fixing this will have an immediately positive impact on our peace.

@cornae
Copy link
Member

cornae commented Jul 4, 2024

Usecases that we want to support with suggested properties design:

No batching, but AJAX (default)

data-pat-auto-suggest="
    ajax-url: https://bla.com/foo.json
  "

Or with an initial batch size that is not 10:

data-pat-auto-suggest="
    ajax-url: https://bla.com/foo.json
    max-initial-size: 20
  "

Batching with arbitrary size

Example for batching enabled with a batch size of 30 and an initial list size of 10:

data-pat-auto-suggest="
    ajax-url: https://bla.com/foo.json;
    ajax-batch-size: 30
  "

Example for batching enabled with a batch size of 20 and an initial list size of 30:

data-pat-auto-suggest="
    ajax-url: https://bla.com/foo.json;
    ajax-batch-size: 20
    max-initial-size: 30
  "
  1. The presence of the property ajax-batch-size already indicates that batching is desired, analogue to how the presence of the property ajax-url indicates that AJAXing is desired. ajax-batch-size would be the only extra property needed for now. The default value of this property is 0.

Not for now

  1. The property max-initial-size is not required right now, but might likely be required in the future, because tuning initial size and follow-up batch size individually is a common UID best practice to optimise the user experience.
  2. The suggested properties design above does not facilitate a use case for having batching on the search result, but no batching on the initial list. I don't see use cases for this at the moment, but they might arise in the future. Should such a use case pop in the future, then extra properties such as ajax-batch-size-search and ajax-batch-size-initial could be added later.

@thet thet force-pushed the scrum-1638-batching branch 3 times, most recently from eecb315 to 4e1b25b Compare December 16, 2024 22:12
@thet
Copy link
Member Author

thet commented Dec 16, 2024

@reinhardt ready for review!

@thet thet requested a review from reinhardt December 16, 2024 22:14
@thet thet marked this pull request as ready for review December 16, 2024 22:14
src/pat/auto-suggest/documentation.md Outdated Show resolved Hide resolved
src/pat/auto-suggest/documentation.md Outdated Show resolved Hide resolved
src/pat/auto-suggest/auto-suggest.js Outdated Show resolved Hide resolved
@thet thet requested a review from reinhardt December 17, 2024 15:54
This PR introduces three new options for that:
max-initial-size: Defines the batch size for the initial request (default: 10).
ajax-batch-size: Defines the batch size for subsequent requests (default: 10).
ajax-timeout: Defines the timeout in milliseconds before a AJAX request is submitted. (default: 400).

Ref: scrum-1638
@thet thet force-pushed the scrum-1638-batching branch from 4c448c3 to 1323eaa Compare December 17, 2024 18:04
@thet
Copy link
Member Author

thet commented Dec 17, 2024

@reinhardt finally implemented the specs as defined (did not carefully enough read them... :/ )

However, there is one case not covered by the specs - loading all items at once on the first page. Is this a use case we want to support?

Currently the code supports it:

  • If you set max-initial-size to 0 all the items will be loaded on the initial page.
  • On the other hand ajax-batch-size set to 0 will disable batching and only show the first page.

I think allowing to load all the items from the server at once without defining the max-initial-size to some ridiculous high number could be useful. Maybe a -1 would be better?
@cornae any input on that? If you want, we can discuss that tomorrow or handle it sometime in the future - it's not documented, so this behavior doesn't exist officially.

@cornae
Copy link
Member

cornae commented Dec 17, 2024 via email

@reinhardt
Copy link
Contributor

I don't have any strong opinions about any of it. I was just wondering why it was different than described.

That said, it might be a good idea to have a limit on the number of results. The reason we are working on this is, after all, that we got in trouble because there were too many entries.

On the other hand, a max-initial-size equal to 0 is a degenerate case - why would you want to do ajax but not be interested in any results? So I'm also fine with giving it a special meaning. You shouldn't set it to 0 without knowing what you're doing anyway.

@thet
Copy link
Member Author

thet commented Dec 18, 2024

According to a chat with @cornae we can keep this PR as it is. Also max-initial-size set to 0 and loading all elements, as long this is not documented.
We might improve it further in the future, if needed.

@thet thet merged commit 81a807a into master Dec 18, 2024
1 check passed
@thet thet deleted the scrum-1638-batching branch December 18, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants