Skip to content
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.

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

Move code for accessible autocomplete component into app #1183

Merged
merged 19 commits into from
Apr 26, 2023

Conversation

MuriloDalRi
Copy link
Contributor

This component was removed in version 18 of govuk_publishing_components alphagov/govuk_publishing_components#1038

We need to move the code into the app so that we can update govuk_publishing_components and other dependencies.

https://trello.com/c/egaF1kjo/3110-replace-accessible-autocomplete-for-content-data-admin

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@andysellick andysellick force-pushed the accessible-autocomplete branch from 751b864 to d14b007 Compare March 30, 2023 07:53
@MuriloDalRi MuriloDalRi marked this pull request as draft March 31, 2023 09:28
@MuriloDalRi MuriloDalRi force-pushed the accessible-autocomplete branch 8 times, most recently from b8b2f01 to ca5b5f7 Compare April 3, 2023 15:38
@robinjam robinjam force-pushed the accessible-autocomplete branch 5 times, most recently from a2987e7 to 225932c Compare April 6, 2023 16:07
@MuriloDalRi MuriloDalRi marked this pull request as ready for review April 11, 2023 08:17
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Looks good so far, needs a couple of minor changes.

@robinjam robinjam force-pushed the accessible-autocomplete branch from 5f9979d to 9a84ca0 Compare April 20, 2023 13:11
MuriloDalRi and others added 12 commits April 25, 2023 18:18
This component was removed in version 18 of govuk_publishing_components alphagov/govuk_publishing_components#1038

We need to move the code into the app so that we can update govuk_publishing_components and other dependencies.

https://trello.com/c/egaF1kjo/3110-replace-accessible-autocomplete-for-content-data-admin
This file is not needed now that we run CI in GitHub Actions.

Jenkins CI also fails because it's on an old version of NodeJS that isn't supported by some dependencies.
Update media queries for mobile

Replace missing $white Sass variable

Replace missing $black Sass variable

Replace missing $grey Sass variables

Replace govuk-focusable-fill deprecated mixin

Replace incorrect govuk-colour Sass references
This is now possible because we've update govuk_publishing_components
This defines what linker should be used for installing Node
packages.

For more information see https://yarnpkg.com/configuration/yarnrc#nodeLinker
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed in 0s 385ms
➤ YN0000: ┌ Fetch step
➤ YN0013: │ jquery@npm:3.6.3 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed in 0s 806ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed in 1s 992ms
➤ YN0000: Done in 3s 230ms

The following files are also generated but are not included as per
https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored:

- .yarn/cache/
- .yarn/install-state.gz
We no longer use jQuery (although `$()` appears to be an alias of `document.querySelector()` for some reason)
This code was never actually used in this app:

The `track` function is only called if `data-track-category` and `data-track-action` are both set on the <select> element. These attributes would be set in `app/views/components/_accessible_autocomplete.html.erb` if a `data_attributes` local is passed in. I've had a look through all the views where we include this partial, and none of them pass any locals when rendering the partial.

Given this, I'm going to remove the tracking code, which also allows us to remove the entire `onConfirm` function (as the remainder of the function is a workaround for a bug caused by the fact that we override `onConfirm`).
@robinjam robinjam force-pushed the accessible-autocomplete branch from 6601c4c to 105997a Compare April 25, 2023 17:20
Fixes an issue where the default/empty option has a different height from the other options, making it appear not to be selectable
@robinjam robinjam force-pushed the accessible-autocomplete branch from 105997a to c2ccde6 Compare April 25, 2023 17:25
@robinjam robinjam requested a review from andysellick April 26, 2023 14:16
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Changes look good 👍

Might be worth seeing if any of the commits could be squashed together, but not a blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants