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]

Just want to check that I'm reading this correctly – this function is effectively doing the same thing as before, it's just using slightly neater syntax and allowing for this.templateInputValue(entry) to return false without trying to treat false as a string?

[ediblecode]

Correct, except that:

allowing for this.templateInputValue(entry) to return false

is more broadly "this.templateInputValue(entry) to return something falsey" - and this is the key, it allows us to return undefined from templates.inputValue:

templates: {
        inputValue: function(suggestion) {
          return suggestion && suggestion.Title;
        }
}

which you have to do if you're providing an inputValue function, because it is called when the plugin loads, so not handling an undefined suggestion argument would result in an Exception.

[36degrees]

This doesn't seem quite right to me, because we're passing a known value and an array containing exactly that known value to a function that's meant to test whether the value is in the array. What we really seem to be testing here is whether this.templateInputValue returns a value that matches or not, so should we just be calling that directly?

I'm not sure isQueryAnOption really describes what's actually going on here either. Is there a better way to describe this?

[ediblecode]

we're passing a known value and an array containing exactly that known value to a function that's meant to test whether the value is in the array

we are, but we want to use the value returned from templateInputValue as you rightly point:

What we really seem to be testing here is whether this.templateInputValue returns a value that matches or not, so should we just be calling that directly?

Fair point, the condition would then become something like this, which is definitely much simpler:

const isQueryAnOption = this.templateInputValue(defaultValue).toLowerCase() === defaultValue;

I think I was trying to re-use the existing isQueryAnOption function as it was already doing what we want.

I'm not sure isQueryAnOption really describes what's actually going on here either.

I guess the behaviour here is whether we should show the default value as an option on focus. So maybe, showDefaultValueAsOption or something? The state initialisation would then become:

options: showDefaultValueAsOption? [defaultValue] : [],

which seems to make sense!

[36degrees]

Is there a way to write these tests that makes it more explicit what's going on?

They currently feel like they're relying on a side effect of the way the inputValue template is implemented in each one, and whether it's dealing with a string or an object. It's not clear to me how the difference is described by the test description, or why the different functions should give different results.

[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