Fix custom templates issue with defaultValue

[ediblecode]

Fixes #424

[danielnaab]

I've encountered this issue as well - is there anything I could help with to get this merged?

[ediblecode]

@danielnaab not sure there is. Have you seen #430? It says they'll prioritize UK public sector (which includes me) so hoping it can go out as part of #433 but I haven't heard anything

[36degrees]

Hi @ediblecode,

We've been looking at this to try and review it for you, but are struggling to get our head around what the correct behaviour should be without a more realistic example. Can you talk us through your use case and what you expect to happen? When would you set a defaultValue that is not one of the options?

Thanks!

[ediblecode]

Thanks for getting back to me @36degrees.

We're using it for a site search typeahead (rather than a list of finite options for a form). The suggestions are requested from a server API endpoint using a function passed to the source prop. Which means defaultValue isn't one of the options, as there aren't any when you first focus.

This is how we're using it if it helps: https://github.com/nice-digital/global-nav/blob/master/src/Header/Search/Autocomplete/Autocomplete.jsx#L123.

Hopefully that, in tandem with the codepen in the issue I raised might be a bit clearer but if not let me know and I'll try add some more details. Thanks

[36degrees]

We're using it for a site search typeahead (rather than a list of finite options for a form). The suggestions are requested from a server API endpoint using a function passed to the source prop. Which means defaultValue isn't one of the options, as there aren't any when you first focus.

Ah, OK, so then when you display the search results page you're using defaultValue to fill in the terms the user searched for?

I appreciate at the same time that the current behaviour (displaying undefined) is definitely not desirable, I'm also wary that we're stretching the component to do things that it was never really designed to do – we're guilty of this too, as we also use it to power the search on the Design System! – so I want to be really careful that we don't start changing the component in ways that negatively affect its primary use case, which is selecting a pre-defined value from a finite list of options.

The expected behaviour here seems a little undefined – what should happen when you focus the input? Given we're in a search context, I think I'd expect the search to be run again and to see the same results that I saw before I submitted the page, not an empty list.

I'll review the code as it stands at the moment with this in mind, and let's see where we get to.

[36degrees]

I think having reviewed your proposed changes again there's something odd about the fact that we're relying (I think?) on the inputValue function being passed a string if the current value of the textbox was either entered by the user or was the defaultValue, versus an object if it's a result that's come from the source function, and using that to decide whether the thing should be shown in the autocomplete?

The fact that inputValue could be passed either seems problematic.

[ediblecode]

Thanks for the review, I'll have a look at the specific feedback. And to answer some other points:

so then when you display the search results page you're using defaultValue to fill in the terms the user searched for?

yes, that's right.

I'm also wary that we're stretching the component to do things that it was never really designed to do

yup, fair enough. Do you think this legit within scope? It feels to me, that because source, templates.inputValue and templates.suggestion all take functions, that this use case in tandem with defaultValue seems plausible, BUT making changes to specifically support site search might be a step too far.

The expected behaviour here seems a little undefined – what should happen when you focus the input?

Very, very good point. I think you're right actually that the expected behaviour should maybe be to display suggestions for the term on focus, and that is new feature I think, specific to site search - I can't see a way to get suggestions to appear on focus based on the current options.

Showing suggestions for search on focus is certainly a common paradigm for search engines. I understand that is different to the current behaviour within accessible-autocomplete though, so would be a new feature. Maybe an option of showSuggestionsOnFocus or something - I'm not sure it would be right to switch that behaviour based on templates.inputValue being a function rather than a string. And also that feels specific to site search so probably a no-go!

So in light of that, I think the expected behaviour would be for it to show no options on focus. I can't see any other way round that? Unless we change the behaviour so the focus doesn't just show [defaultValue] as it does now, but loads suggestions - but that feels like a breaking change and the wrong approach.

[36degrees]

In #424 you suggested another approach:

OR we change the options.map call within the render, to only render options where the result of this.templateSuggestion(option) is not falsey.

Did you explore this at all? Effectively, I think we'd instead be trying to solve a slightly simpler bug:

if you return empty string then you still get an 'empty' suggestion

This would then allow you to address this in your own suggestion template function, rather than requiring changes to the autocomplete logic? I can't think of a situation where 'gracefully handling' suggestion returning something falsey would cause a problem.

[ediblecode]

Interesting, so I did. And I even suggested option 2 was better didn't I?! Thanks, past me. I'll look at that then, might be more elegant. I'm sure there was a reason why I went down the other route, but pushing the logic to the consumer as to what they return from the function makes sense if that works.

[colinrotherham]

Hey @ediblecode, long time no see (took a little squinting at your profile photo!)

We've given this PR a milestone and I've rebased it with main to help us review it