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.

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

Jump to bottom

AP-5170: Country accessible autocomplete #7166

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

AP-5170: Country accessible autocomplete #7166

wants to merge 15 commits into from
+100 −48

Conversation

agoldstone93
Copy link
Contributor

@agoldstone93 agoldstone93 commented Sep 10, 2024 •
edited
Loading

Replace existing search for countries in non-UK home address with accessible autocomplete

Checklist

Before you ask people to review this PR:

  • Tests and rubocop should be passing: bundle exec rake
  • Github should not be reporting conflicts; you should have recently run git rebase main.
  • The standards in the Git Workflow document on Confluence should be followed
  • There should be no unnecessary whitespace changes. These make diffs harder to read and conflicts more likely.
  • The PR description should say what you changed and why, with a link to the JIRA story.
  • You should have looked at the diff against main and ensured that nothing unexpected is included in your changes.
  • You should have checked that the commit messages say why the change was made.

@agoldstone93 agoldstone93 force-pushed the testing-country-accessible-autocomplete branch from 080abe8 to de2251d Compare September 24, 2024 12:32
@agoldstone93 agoldstone93 force-pushed the testing-country-accessible-autocomplete branch from 37d9ea9 to fc002b1 Compare October 1, 2024 16:24
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Oct 1, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@agoldstone93 agoldstone93 force-pushed the testing-country-accessible-autocomplete branch from fc002b1 to 0c7b2c9 Compare November 7, 2024 08:16
@agoldstone93 agoldstone93 force-pushed the testing-country-accessible-autocomplete branch from 0c7b2c9 to f4a084b Compare November 28, 2024 08:31
@agoldstone93 agoldstone93 changed the title Testing country accessible autocomplete Country accessible autocomplete Nov 28, 2024
@agoldstone93 agoldstone93 marked this pull request as ready for review November 28, 2024 13:11
@agoldstone93 agoldstone93 requested a review from a team as a code owner November 28, 2024 13:11
naseberry
naseberry reviewed Nov 28, 2024
View reviewed changes
app/assets/stylesheets/accessible-autocomplete.scss Outdated
Comment on lines 1 to 9
.clear-search {
margin-top: 50px;

&-error {
margin-top: 90px;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

RWD: would it be ideal to use the govuk spacing mixin and values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The values only go up to 60px, sadly

I could do something funky like have the 50px always there and add the extra 40px when there's an error, by either using variables or adding the margin to two separate elements, but they didn't seem right. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing funky.
Funky is not standard practice and it always becomes a pain.

As the max value is only 60px. I would suggest we test the responsiveness of your units.

naseberry
naseberry reviewed Nov 28, 2024
View reviewed changes
app/javascript/src/accessible-autocomplete.js
@@ -0,0 +1,42 @@
import accessibleAutocomplete from 'accessible-autocomplete'
Copy link
Contributor

@naseberry naseberry Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Componentization improvement: These functions would only work with one accessible component per page, that works for well for our current page layout, could this be made better to by supporting multiple autocompletes on single page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@naseberry Do we need this? I think we'll only ever have one on a page. If so, I'm not sure exactly what you mean or how to achieve it

@agoldstone93 agoldstone93 changed the title Country accessible autocomplete AP-5170: Country accessible autocomplete Dec 3, 2024
@agoldstone93 agoldstone93 force-pushed the testing-country-accessible-autocomplete branch from 543f510 to b1391d8 Compare December 5, 2024 13:39
@agoldstone93 agoldstone93 force-pushed the testing-country-accessible-autocomplete branch from b1391d8 to f08dd2d Compare December 17, 2024 13:37
@agoldstone93
add accessible autocomplete, styling, update form and view wip
f1bd858
@agoldstone93
Add blank box option
36774f5
@agoldstone93
Add back clear search link
a0b16fa
@agoldstone93
Update accessible autocomplete to v3.0.1
2c50b19
@agoldstone93
Reorder validation and add class to select
e9542ab 
Reorder validation to change the order it's displayed.
Add class to connect accessible autocomplete using class
rather than existing id as this changes when there is
an error.
@agoldstone93
Updates to display
f873629 
Clear search not shown when js is off.
Add 'govuk-body' class to update font.
Clear search working when js is off.
Don't run autocomplete js on other pages
@agoldstone93
Fix failing tests
2ae55aa 
Update tests to reflect changes
@agoldstone93
Fix vertical alignment of 'Clear search'
06bc770 
Fix vertical alignment when there is an error on the page
@agoldstone93
Add border colour when error on autocomplete
daf1947 
Add class to autocomplete when there is an error. Seems to change color
back to black once the user starts typing in an answer. This may not
be preferable.
@agoldstone93
Update clear search top margin to work correctly
5f9b1bd
@agoldstone93
Remove unused 'no results found' code
3e2645e
@agoldstone93
Make changes flagged in code review
4821d67 
Add and remove existing class to show and hide elements
Move text into locales files
@agoldstone93
Update autocomplete width
6165609 
Following responsive web design, this removes unnecessary extra
spacing when on non-desktop devices
@agoldstone93
Fix bug to fully remove selected country
bd3e0e7
@agoldstone93 agoldstone93 force-pushed the testing-country-accessible-autocomplete branch from f08dd2d to bd3e0e7 Compare December 23, 2024 11:44
@agoldstone93
Refactor javascript
f86e737 
Update javascript code so accessible autocomplete component is reusable
@agoldstone93 agoldstone93 force-pushed the testing-country-accessible-autocomplete branch from ed0d43a to f86e737 Compare December 23, 2024 13:53
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@naseberry naseberry naseberry left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@agoldstone93 @naseberry