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

Autocomplete input with prefilled v-model causes unnecessary call to options method #657

Closed
mweghorst opened this issue Apr 18, 2023 · 16 comments
Assignees
Labels
🐛 bug Verified bug by team 🟡 priority-2 2. Medium priority issue Pro Pertains to FormKit Pro 🚀 release-ready Feature or fix is complete and on an upcoming release branch
Milestone

Comments

@mweghorst
Copy link

Reproduction

https://formkit.link/09482f6f5095df7c99617c4f1039f742

Describe the bug

When using a FormKit autocomplete input with v-model, the options loader is called whenever an option is selected. The query set to the selected option, which causes the list of options to contain only the selected option after a reset. This only happens if the v-model is a ref that has an initial value. If the v-model is not a ref, or if it is an empty ref, the problem does not occur.

You can see that the result of the options call has only 1 result in this video: https://www.loom.com/share/d426a4deef2b4ccfa6e06c4dce92338a. The network tab isn't visible unfortunately, but a call to this url is made after selecting The Goblet of Fire: https://api.themoviedb.org/3/search/movie?query=Harry%20Potter%20and%20the%20Goblet%20of%20Fire

Environment

• OS: MacOS
• Browser Chrome

@mweghorst mweghorst added ⛑ Needs triage The issue has not yet been examined by the FormKit team. 🐛 bug-report Bug is reported, but not verified by team labels Apr 18, 2023
@sashamilenkovic sashamilenkovic added 🐛 bug Verified bug by team 🟡 priority-2 2. Medium priority issue Pro Pertains to FormKit Pro and removed 🐛 bug-report Bug is reported, but not verified by team ⛑ Needs triage The issue has not yet been examined by the FormKit team. labels Apr 18, 2023
@sashamilenkovic
Copy link
Contributor

@mweghorst Hey! If you check out the next tag in the playground (so you're pulling down 0.111.8-4cc3687). Can you confirm that it's working correctly now?

@sashamilenkovic
Copy link
Contributor

Also, your issue #656 should be resolved as well.

@mweghorst
Copy link
Author

which causes the list of options to contain only the selected option after a reset

This part is indeed fixed in @next, because the autocomplete does a fresh call to the options method. However, upon selecting one of the options a call is still made to the options method with the selected value as the query (i.e. to https://api.themoviedb.org/3/search/movie?query=Harry%20Potter%20and%20the%20Goblet%20of%20Fire after clicking on The Goblet of Fire). As a result, a limited set of results (based on the query) is visible for a very short time. I can't seem to upgrade the Playground to Pro@next, so I'm sharing a video of my local dev environment: https://www.loom.com/share/d3690ba106204bc1a57f7ac50aa2678f

Also, your issue #656 should be resolved as well.

Yes that issue is resolved, thanks!!

@sashamilenkovic
Copy link
Contributor

@mweghorst Gotcha. It makes sense that another request is made to your "optionsLoader" since removing the selected value does "change" the search value to empty string. It would be better if that weren't the case. I can continue taking a look into this.

@mweghorst
Copy link
Author

mweghorst commented Apr 18, 2023

Just to be sure I expressed myself correctly: the issue is not so much the post-reset request to searchMovies() with an empty query, but the post-select request to searchMovies() with the query set to the selected value/label. The result of the latter is never used I think.

Additionally and possible related: when clearing the "Titanic" value on https://formkit.link/09482f6f5095df7c99617c4f1039f742, then immediately 2 searchMovies() requests are made.

@sashamilenkovic
Copy link
Contributor

@mweghorst Hey sorry for the delay. I just published a new version of the pro package at the latest tag, https://www.npmjs.com/package/@formkit/pro/v/0.111.9. When you have time, can you take a look and let me know where we stand on your issues?

@sashamilenkovic
Copy link
Contributor

@mweghorst One thing to notice, I changed the behavior of open-on-click. Clicking the "removeSElection" button will not open the listbox like it used to (I feel that behavior should be handled by a separate prop maybe in the future).

@mweghorst
Copy link
Author

mweghorst commented Apr 20, 2023

@sashamilenkovic Upon selecting an item, a request is still being made to the options function with the label of the selection as the query parameter (i.e. when selecting Goblet of Fire, a request to https://api.themoviedb.org/3/search/movie?query=Harry%20Potter%20and%20the%20Goblet%20of%20Fire). The results of that call are never used because a new request is made when (re-)opening the autocomplete, right?

Clicking the "removeSelection" button will not open the listbox like it used to

This is unfortunately breaking what I want to accomplish, although I understand why you added it. What I'd like to do is to reset the selection and open the dropdown on the click event. I added a feature request (including a playground link) for this: https://github.com/orgs/formkit/discussions/658

@sashamilenkovic
Copy link
Contributor

@mweghorst Yeah, sorry for not addressing the "request is still being made to the options function with the label of the selection as the query parameter" issue. So here are my thoughts:

You're right that we are essentially making duplicate requests, one when the selection is made and one when the autocomplete is opened. However, if we do not make the initial request when the option is selected, when you reopen the autocomplete, you're going to see the "old" options first, then the request will be made with the new search value, so you'll always see the old options changed out for new ones. We could potentially prevent the autocomplete from being expanded until that request is made, but that seems worse (to me, maybe I'm wrong).

I think what might be better is to allow the request to be made with the selected option's label initially, but then have a cache from preventing a second request from firing when reopening the autocomplete if the previous search value is equivalent to the current search value.. maybe something like this: https://formkit.link/fef695f9bfc8c46f4a3f1c5ec38f7d50

Potentially we could make this sort of caching part of the feature set itself, but I feel it's maybe a bit too opinionated (considering the initial search and the search when expanding could produce different results from the backend).

@sashamilenkovic sashamilenkovic self-assigned this Apr 20, 2023
@mweghorst
Copy link
Author

However, if we do not make the initial request when the option is selected, when you reopen the autocomplete, you're going to see the "old" options first

Is this not only an issue when using selection-appearance="text-input"? When using "option" the autocomplete can only be reopened with an empty query I think.

you'll always see the old options changed out for new ones

This is exactly what is happening for my input right now but the other way around if you understand what I mean. You'll see the results of query=selectedLabel being replaced by the results of query=null: https://www.loom.com/share/d3690ba106204bc1a57f7ac50aa2678f

@sashamilenkovic
Copy link
Contributor

@mweghorst Okay so in regards to your loom video (appreciate the visual evidence btw), have you updated to the latest version? Clicking "removeSelection" should no longer open the listbox (until we implement a prop that allows that behavior).

@mweghorst
Copy link
Author

Yes, in the latest version the listbox is indeed not opened when clicking removeSelection. However (the Loom video doesn't show this very well), I want the Listbox to opened on removal because I want the autocomplete to behave more like a dropdown: https://github.com/orgs/formkit/discussions/658. The optimal flow for us would be:

  1. Open the Listbox onclick
  2. Make a request to options with an empty query
  3. Make subsequent requests to options when typing a query
  4. Select a value and close the listbox. No extra requests necessary at this point
  5. Clear all listbox suggestions in the background
  6. Reset and re-open the listbox (that is now empty) when clicking anywhere on the input
  7. Make a new request to options with an empty array

Do you think the clearOnClick will be added? And if so, any idea what the timeframe would be? If it's rather soon I'll wait and retry this flow then.

@sashamilenkovic
Copy link
Contributor

@mweghorst Got it. I'll try to get this implemented by early next week.

@mweghorst
Copy link
Author

@mweghorst Got it. I'll try to get this implemented by early next week.

Awesome!

@sashamilenkovic
Copy link
Contributor

sashamilenkovic commented Apr 25, 2023

@mweghorst Hey! Finally got around to working on this: https://formkit.link/519e2b1a81ea636ed41e398a13df6021. I added the "clearOnClick" prop we talked about, do you want to give this a look when you get a chance? https://www.npmjs.com/package/@formkit/pro/v/0.111.13-390d1a3 is the version on the next tag.

@mweghorst
Copy link
Author

@sashamilenkovic Sorry I didn't get around to this earlier but I've just tested this and it's working great! There is only one thing I noticed but it's very minor: when typing a query (i.e. Star Wars), selecting an entry, and then clear-on-clicking it the suggestion box shortly shows/flickers Star Wars movies before loading the Harry Potter ones. But again this is very minor and not something I'm even sure could be fixed

Many thanks for this!

@luan-nk-nguyen luan-nk-nguyen added the 🚀 release-ready Feature or fix is complete and on an upcoming release branch label Aug 10, 2023
@luan-nk-nguyen luan-nk-nguyen added this to the Beta 18 milestone Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Verified bug by team 🟡 priority-2 2. Medium priority issue Pro Pertains to FormKit Pro 🚀 release-ready Feature or fix is complete and on an upcoming release branch
Projects
None yet
Development

No branches or pull requests

4 participants