Skip to content

Fix custom templates issue with defaultValue #425

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+46 −9
Open

Fix custom templates issue with defaultValue #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21b6550
Add failing test for expected scenario
ediblecode Feb 27, 2020
3921fc4
Fix inputValue template problem with defaultValue
ediblecode Feb 27, 2020
fdc205e
Rebuild dist
ediblecode Feb 27, 2020
8c424e1
Fix linting issue
ediblecode Feb 27, 2020
8baad85
Rebuild dist
ediblecode Feb 27, 2020
4b55ff3
Update changelog
ediblecode Feb 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

### Fixes

- [Pull request #425: Fix using templates and defaultValue resulting in an empty option on focus](https://github.com/alphagov/accessible-autocomplete/pull/425)
- [Pull request #415: Make React bundle work server-side in a NodeJS environment](https://github.com/alphagov/accessible-autocomplete/pull/415)

## 2.0.2 - 2020-01-30
Expand Down
2 changes: 1 addition & 1 deletion dist/accessible-autocomplete.min.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/accessible-autocomplete.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/lib/accessible-autocomplete.preact.min.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/lib/accessible-autocomplete.preact.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/lib/accessible-autocomplete.react.min.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/lib/accessible-autocomplete.react.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

9 changes: 6 additions & 3 deletions src/autocomplete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,15 @@ export default class Autocomplete extends Component {
constructor (props) {
super(props)

const { defaultValue } = props
const isQueryAnOption = defaultValue.length > 0 ? this.isQueryAnOption(defaultValue, [defaultValue]) : false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


this.state = {
focused: null,
hovered: null,
menuOpen: false,
options: props.defaultValue ? [props.defaultValue] : [],
query: props.defaultValue,
options: isQueryAnOption ? [defaultValue] : [],
query: defaultValue,
validChoiceMade: false,
selected: null,
ariaHint: true
Expand Down Expand Up @@ -95,7 +98,7 @@ export default class Autocomplete extends Component {
}

isQueryAnOption (query, options) {
return options.map(entry => this.templateInputValue(entry).toLowerCase()).indexOf(query.toLowerCase()) !== -1
return options.some(entry => (this.templateInputValue(entry) || '').toLowerCase() === query.toLowerCase())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

componentDidMount () {
Expand Down
33 changes: 33 additions & 0 deletions test/functional/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,39 @@ describe('Autocomplete', () => {
expect(autocomplete.state.options[0]).to.equal('France')
expect(autocomplete.state.query).to.equal('France')
})

it('is prefilled with a simple custom inputValue template', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

autocomplete = new Autocomplete({
...Autocomplete.defaultProps,
defaultValue: 'France',
id: 'test',
source: suggest,
templates: {
inputValue: suggestion => suggestion,
suggestion: suggestion => `<a href="/search?q=${suggestion}">${suggestion}</a>`
}
})

expect(autocomplete.state.options.length).to.equal(1)
expect(autocomplete.state.options[0]).to.equal('France')
expect(autocomplete.state.query).to.equal('France')
})

it('isn\'t prefilled with a custom inputValue template that returns a non-match', () => {
autocomplete = new Autocomplete({
...Autocomplete.defaultProps,
defaultValue: 'France',
id: 'test',
source: suggest,
templates: {
inputValue: suggestion => suggestion && suggestion.title,
suggestion: suggestion => suggestion && `<a href="${suggestion.href}">${suggestion.title}</a>`
}
})

expect(autocomplete.state.options.length).to.equal(0)
expect(autocomplete.state.query).to.equal('France')
})
})
})

Expand Down