Add fetchInitialResults prop to ContentSearch
#124
Conversation
This new prop allows control over the focus state on ContentSearch. It will trigger a fetch just by focusing the search field, making the user aware of what they might be selecting from. To keep previous expectations the default value is `false`.
This makes the ContentSearch provide results when focusing the search field for all pickers without any selected items.
fabiankaegy
added
[Type] Enhancement
|
Hey @xipasduarte, firstly I want to thank you for taking the time for both, opening a PR and also taking the time to go over and clean some code! Thanks! I'm not sure if Helen's intention was to actually perform the query on focus or rather show some results on focus. I've recently participated in a project which has over 200,000 published articles in which any search took over 10 seconds which makes me wary of adding this behavior since based on payload size and network conditions, the UI can look unresponsive for no apparent reason. While I understand this is very much an edge case, I'd say this should just prompt the search on mount so results are ready for whenever the user decides to focus the input, it just gets the results. (this would mean a rename on the new prop). I think it's kind of safe to assume that, if the block is selected (so this component is mounted), you're likely to perform actions that might lead to a search that would benefit from this new prop and behavior. What are your thoughts on this? Also interested to know about the opinions of @fabiankaegy and @tlovett1 on this! Thanks again! |
|
Hi @Antonio-Laguna, I'm happy to help and hope I can keep at it 🙂. I agree that there is an expectation of using the component once it is selected. We also have a focus on the search field when adding the block, which enforces this expectation. So, regardless of the edge case, I think it's a great idea to "warm up" with the initial results rather than fetching on focus. Any suggestion for the prop rename? Maybe something like I'll leave some time for others to give feedback before going forward with any changes. I'm going to just test and see if the presence of the search field, even if empty, is affecting the performance of the query. I expected this would be like fetching the first page of any post type archive, but if there are issues we can remove the query arg |
|
@xipasduarte I think |
This is a new approach to the solution where the results may be prefetched to display when focusing. This is only valid once the component mounts.
|
I made the changes to consider From what I could gather the first fetch will be an empty search field, so this means it is analogous to the first page of the archive and should be fast. (If anyone has a particularly hard example, please run these with it to double check.) Given this, should we expose this prop in the |
xipasduarte
changed the title
fetchOnFocus prop to ContentSearchfetchInitialResults prop to ContentSearch
|
Hey @xipasduarte ! Thanks for doing this again! I've flagged a minor typo but other than that it's looking good! I agree with it being added to |
Extende the prop from `ContentSearch` to be controllable by the `ContentPicker` interface. Set the default value to `true` to enhance UX.
|
@Antonio-Laguna I've made the correction and extended the Is it possible to enable the tests to run so as to see if everything passes? Let me know if anything else needs a little more work 🙂 |
There was a problem hiding this comment.
Thank you @xipasduarte for your patience on this and for working on it in the first place. I just tested this and made one small update to not have it enabled by default. Besides that this is ready to go and approved from my end :)
Description of the Change
This new prop allows control over initial state for the queries in
ContentSearch. Itwill trigger a fetch when mounting the component and cache the response for display by focusing the search field, making the user aware of what they might be selecting from.
A new state was added to keep track if the search field is focused. This allows for better control of the results area display. To keep previous expectations the default value is
false.Then, on the
ContentPickercomponent extend the same prop with a default value oftrueto enhance UX.Closes #43
Alternate Designs
Another alternative was considered for the UX on
ContentPicker, but the logic is fairly simple to fix if want to. See the issue for more details.Possible Drawbacks
I thought about adding an option for the user to specify if they want this fetch to happen on every item count and the effect it would have. I believe that this other prop could be added later without any drawbacks. The only issue might be from a UX stand point, if it becomes preferable to have
fetchInitialResultsfor all item counts inContentPicker.Verification Process
example/hello-worldwhich has a post picker following the expected scenarios:fetchInitialResults = truethe results of the query should be displayed.fetchInitialResults = falsethe current behavior should occur, with fetch.ContentPickeris used it should fetch initial results when no items existcontent.length === 0. Otherwise the current behavior is expected.fetchInitialResults = falseas a default the current working logic is kept forContentSearch.Checklist:
Changelog Entry
Credits
This new prop allows initial results to be fetched when mounting
ContentSearch. These results will display when focusing the search field, making the user aware of what they might be selecting from.To keep previous expectations the default value is
falseonContentSearch.Props @helen for detailing an initial approach to solve this UX problem at #43.