Unify APIs for the 'autocomplete' and 'select with search' components #9958
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In March 2023, in #7378, it was decided to make 'multiple select' autocomplete occurrences use the `showAllValues` option by default. This means that all options are displayed as soon as the input is interacted with, without any additional search action required. The PR did not describe why 'single' inputs should not receive the same treatment. It is hypothesised that without showing users the available options (i.e. enabling `showAllValues`) then they will find it more difficult to know which value to search for and select. If we don't change this behaviour, then it means we can't offer feature parity in swapping component usage from 'select with search' over to 'autocomplete' (which is what we're trying to do in https://trello.com/c/pmbL2yId/3411-remove-the-selectwithsearch-component-from-whitehall to improve consistency within Whitehall). Note that the behaviour can still be overridden on a case by case basis as per the example in the YML file: ```yml autocomplete_configuration_options: showAllValues: false ```
It looks as though this used to be used for GA4 tracking, but that was removed in f541cb9. What's left is just the native JS/select `selected` manipulation which you'd expect to be defined within accessible-autocomplete-multiselect itself.
It's not immediately clear how to auto-fix any issues detected by Prettier, nor is there an obvious way of getting Prettier to even tell you what the issues _are_. So adding a script that auto-fixes, which the developer can run manually if needed.
It's a common use case to show a blank option on the select dropdown, and it feels wrong that in each case where that's required, we have to prefix options with `[""]`. It should just be a natively supported feature (allowing us to change the underlying implementation much more easily). This commit carries over the `include_blank` option from the 'select_with_search' component so that we have feature parity between the two components. It also uses the new option on existing 'autocomplete' instances.
"Needs" is a Maslow thing and is going away. This is the only occurrence of those properties in all 'autocomplete' calls and so removing them will simplify delivering feature parity with the 'select with search' component. The [current implementation](https://github.com/alphagov/whitehall/blob/20fcfed3948f218d05b932ee910f91161e245c8f/app/views/components/_autocomplete.html.erb#L23-L25) of autocomplete just merges these properties into a hash that is passed to the 'label' component. Whilst label supports 'bold', it doesn't appear to support 'required', so it's unlikely that was achieving anything: https://components.publishing.service.gov.uk/component-guide/label
This didn't appear to be in use anywhere.
This brings the 'autocomplete' config closer in line with the 'select with search' config, which defines 'heading_size' in the root hash rather than in the `label`. It appears govuk_publishing_components is equally divided, so there is no strong reason to go with one approach vs the other: alphagov/govuk_publishing_components#4641 Note that the 'select_with_search' component makes use of the `GovukPublishingComponents::Presenters::SharedHelper` helper to determine whether the passed 'heading_size' is valid. I did add a test to 'autocomplete' with a view to adding that validation to 'autocomplete', but the test passed without modification, so it appears the component already avoids using invalid values and thus there's no need for us to check for it in Whitehall: ``` test "avoids passing heading size to label component if invalid value" do data = component_data data[:heading_size] = "sausages" assert_select ".govuk-label.govuk-label--sausages", { count: 0 } assert_select ".govuk-label", { count: 1 } end ```
This is a case of YAGNI. It's supported by the 'select' component (which uses it in one place in Whitehall: https://github.com/alphagov/whitehall/blob/514ba658e64b3dd3b5bb12a607e47f3f0d80422c/app/views/admin/worldwide_organisations_main_offices/show.erb#L32) but it isn't used by any select_with_search component. We can always add it back in later if we need it. Removing it simplifies the proposition between 'autocomplete' and 'select with search' (i.e. we don't need to port over the `is_page_heading` behaviour to the 'autocomplete' component if we were to decide to switch over to that component exclusively).
There is no clear convention in govuk_publishing_components as to whether a label should be a string or a hash. Components with string labels: - https://components.publishing.service.gov.uk/component-guide/copy_to_clipboard - https://components.publishing.service.gov.uk/component-guide/password_input - https://components.publishing.service.gov.uk/component-guide/select - https://components.publishing.service.gov.uk/component-guide/search_with_autocomplete Components with hash labels: - https://components.publishing.service.gov.uk/component-guide/file_upload - https://components.publishing.service.gov.uk/component-guide/character_count - https://components.publishing.service.gov.uk/component-guide/input Given we only now have a 'text' subproperty of 'label' for the 'autocomplete' component, the hash is a bit clunky to use and offers no real benefit. So bringing it in line with 'select with search', we're switching to a string, which will make it easier to consolidate the components.
This is (useful) behaviour defined in the 'select with search' component. Adding it into 'autocomplete' will make it easier to consolidate the components.
This is a feature of the 'autocomplete' component that needs carrying over to 'select with search' to consolidate the components.
These don't appear to be used anywhere.
The query caching (added in #1712 and #1728) is important to retain as, according to the linked PRs, it causes considerable savings in page load times for the affected pages. However, in one of the next commits we'll be changing the structure of the returned data to include not only the `text` and `value` of each option, but also its `selected` status. We don't want to cache the `selected` status as that would mean for example we're showing the same "Lead Organisation" selected item across all five "Lead organisations" dropdowns on the "New document" page. So, we're keeping the caching, but we're just going to cache the expensive bit - the database result. The `map` calls are very quick in comparison and don't need to be cached.
The 'autocomplete' component (which uses 'multiple' select) will use the SelectWithSearchHelper class in the next commit. Before it can do so, the helper needs to be able to mark multiple options as "selected" for the autocomplete tests to continue to pass.
To bring the data structures for 'autocomplete' in line with the
data structures for 'select with search', we've:
- Removed the unnecessary layer of `select -> options` so that
it's now just `options`
- Done away with the array style `["Text", "value"]` in
favour of the more expressive
`{ text: "Text", value: "value", selected: true }`
- Updated all the affected code
- NB: I'm not entirely happy with the "selects organisation
if selected in filters" test that's been updated as a result,
as we're now really just testing that we've stubbed a value,
which is a bit meaningless.
This was needlessly different to the 'autocomplete' implementation which takes an array of errors, preventing consolidation of the components. The 'select with search' component was just following what was defined in the 'select' component: https://components.publishing.service.gov.uk/component-guide/select#with_error It 'delegated' the error logic to that component, which is quite a nice pattern. However, the prevailing opinion seems to be that the `error_items` array is the way to go. See elsewhere in Whitehall, e.g. https://github.com/alphagov/whitehall/blob/6b00ce8873698a556ff3db44acf0a45c137f2e1c/app/views/admin/attachments/_form.html.erb#L12 See also the commit message that introduces the `errors_for_input` method, which adds a special method just to support `error_message` for the `input` component, whereas "All other [components] require an array of hashes". NB: `error_id` being a specifyable thing was weird: it didn't appear to be being manually set in application code anywhere. By moving to `error_items`, we've removed that unnecessary bloat. Note the mass switch from `errors_for_input` (which squashes to a string) to `errors_for` (which converts to an array of hashes as expected) The `errors_for_input` class should only really be used by the `input` component (as explained in the commit linked above).
This was referenced Feb 20, 2025
This adds the last bit of feature parity (apart from the two big differences: 'multiple' select support for 'select with search', and 'grouped options' support for 'autocomplete', which will follow in a subsequent PR).
ryanb-gds
approved these changes
Feb 20, 2025
| @@ -9,6 +9,7 @@ | |||
| "lint:js": "eslint --cache --cache-location .cache/eslint --color --ignore-path .gitignore -- \"**/*.js\"", | |||
| "lint:scss": "stylelint app/assets/stylesheets/", | |||
| "lint:prettier": "prettier --cache --cache-location .cache/prettier --cache-strategy content --check -- \"**/*.{js,scss}\"", | |||
| "lint:prettier:fix": "prettier --write \"**/*.{js,scss}\"", | |||
There was a problem hiding this comment.
Hah, I should have done this before now. Thanks
| @@ -4,6 +4,7 @@ | |||
| error_items ||= [] | |||
| aria = error_id if error_items.any? | |||
| select ||= {} | |||
| include_blank ||= false | |||
There was a problem hiding this comment.
Nice, the empty string concatenation has long bugged me
|
|
||
| def cached_taggable_organisations | ||
| Rails.cache.fetch(taggable_organisations_cache_digest, expires_in: 1.day) do | ||
| Organisation.with_translations.order("organisation_translations.name") |
There was a problem hiding this comment.
Some of these queries (this one being a good example) shouldn't really be very slow - if it is it might be a missing index on the translation name. I can very much imagine a MoG change scenario where we put an org live and then have to shell into prod to clear the cache because we can't tag any content to it.
I suppose my question is could we apply the caching a bit more selectively/check we've got the indexes we need? Probably a bit outside the scope of this PR, to be fair
There was a problem hiding this comment.
Yes, I suspected as much too. Digging into the commit history (read 0a2f1b7), it looks like some of the cached queries have a significant impact on page load times, so I didn't want to open that can of worms. Would be ripe for a revisit in a future PR though.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello: https://trello.com/c/pmbL2yId/3411-remove-the-selectwithsearch-component-from-whitehall
This PR is a refactor, to make the data structures used by 'autocomplete' and 'select with search' more alike, to enable us to consolidate the components in a future PR. There are no user facing changes here: all the same existing components are used and there should be no change to look/feel/functionality.
Changes:
SelectWithSearchHelperclass as a result (so that both components have one way of converting the array of hashes into the 'options HTML').multipleparameter.include_blank: trueoption instead of having to prefix an empty stringname.This leaves the components' data structures with one main difference each:
autocomplete_configuration_options, but given that's specific to the accessible-autocomplete-multiselect library, that's not something we'd want to carry over. It also isn't currently used - it's just a documented feature.In the next PR, we'll either switch all 'select_with_search' calls to use 'autocomplete' instead, or switch all 'autocomplete' calls to use 'select_with_search'. Whichever component is left unused will be deleted as part of the same PR. It also makes it easier to swap out the component for a more accessible alternative in future, as we'll only be dealing with one kind of component (and one set of 'parameters' passed to the component) instead of two.
Background
There are currently 17 instances of "autocomplete" and 24 instances of "select_with_search".
Neither the latter PR nor the Trello card explain why ‘autocomplete’ wasn’t used instead (nor indeed makes any mention of ‘autocomplete’ at all). It may have been an attempt to side-step the known problems with accessible-autocomplete component, or it may be that search_with_select was used as a drop in jQuery-less replacement for how Whitehall was using Select2. Some resources Kevin was able to dig up:
In any case, both components look very similar and share a lot of functionality. Here is a key table of differences:
[text, value]{ text:, value: }selected: <value>text:, bold: trueheading_sizeoption to change its sizeis_page_heading: truebooleanerror_itemsarrayerror_message/error_idgrouped_optionsproperty[""] +prefix when specifying blank optioninclude_blank: truebooleanThere is little point in maintaining two highly similar but differently architected components: we should pick only one, and retire the other.
Deployment note
This has been successfully tested on Integration. One thing to note: because the cached objects in TaggableContentHelper have changed, we need to clear the cache with
Rails.cache.clearafter deploy, otherwise users will see the generic "server issue" error. See https://govuk.sentry.io/issues/6317660943/activity/?environment=integration&project=202259&query=is%3Aunresolved%20issue.priority%3A%5Bhigh%2C%20medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=1Follow these steps if you are doing a Rails upgrade.