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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

LG-11082 Refactor Post Office Search to use FullAddressSearch #1174

Merged
merged 8 commits into from
Oct 11, 2023

Conversation

gina-yamada
Copy link
Contributor

@gina-yamada gina-yamada commented Oct 5, 2023

馃帿 Ticket

LG-11082 Refactor identity-site to use FullAddressSearch

馃洜 Summary of changes

  1. Increased version of @18f/identity-address-search from 3.0.0 to 3.1.0 3.1.1
  2. Updated Post Office Search to use FullAddressSearch rather than AddressSearch

馃摐 Testing Plan

  • Step 1 Pull down this branch, run npm i to install latest version of @18f/identity-address-search
  • Step 2 Edit _config.yml. Change show_po_search from false to true, start the server
  • Step 2 Go to http://localhost:4000/help/verify-your-identity/verify-your-identity-in-person/find-a-participating-post-office/
    (Api does not work yet. It is being refactored in another ticket)

馃摳 Screenshots

Before- using AddressSearch

Screenshot 2023-10-05 at 8 35 45 AM

Screenshot 2023-10-05 at 8 36 11 AM

After- using FullAddressSearch

Screenshot 2023-10-10 at 1 14 00 PM

Screenshot 2023-10-10 at 1 22 39 PM

Screenshot 2023-10-10 at 1 22 49 PM

The screenshots below are before the text update was added (address-search v 3.1.0 -- see commit history of this PR). The images below are now outdated as we are using 3.1.1. I am keeping the screenshots in as the additional text was added in the PR out a conversation/review of these images.

Screenshot 2023-10-05 at 8 33 52 AM

Screenshot 2023-10-05 at 8 34 41 AM

Screenshot 2023-10-05 at 8 34 58 AM

@gina-yamada gina-yamada marked this pull request as ready for review October 5, 2023 18:59
@allis-green
Copy link

@gina-yamada can you add the sentence: 'Enter an address to find a Post Office near you.' right on top of the full address component?

@gina-yamada
Copy link
Contributor Author

gina-yamada commented Oct 6, 2023

Enter an address to find a Post Office near you

@allis-green I am working on getting this sentence added. I checked in with Matt- I am not blocking him so will just update the version in this PR once I get the change made in identity-idp and it published. (PR for identity-idp to make this change)

  1. Is a p tag fine? (See screenshots below)
  2. Should the sentence have a period? English and Spanish do not currently have a period but the French translation does. I feel like it should be consistent- either they all end with a period or they all don't. What change should I make?

(Also, only look at section in red- that is the component/piece that will display in the Help Center)

Screenshot 2023-10-06 at 9 41 24 AM

Screenshot 2023-10-06 at 9 41 44 AM

Screenshot 2023-10-06 at 9 42 49 AM

@allis-green
Copy link

@gina-yamada I think that a p-tag works great for that string of text. I'll reference the style guide re: your question about ending the sentence with a period.

@allis-green
Copy link

@gina-yamada I couldn't find anything in the style guide about when not to use a period for paragraph text, so I'm just going to make the call that we should be using a period for that string.

@gina-yamada
Copy link
Contributor Author

gina-yamada commented Oct 10, 2023

I updated the version of @18f/identity-address-search from 3.0.0 to 3.1.1 since this PR was approved. The text Enter an address to find a post office near you will no be displayed on the Full Address Search Post Office form in the help center. Only when identity-idp translations gets merged in, will all translations end with a period.

Screenshot 2023-10-10 at 1 14 00 PM

Screenshot 2023-10-10 at 1 22 39 PM

Screenshot 2023-10-10 at 1 22 49 PM

Copy link
Contributor

@allthesignals allthesignals left a comment

Choose a reason for hiding this comment

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

LGTM

const field = getField();
expect(field).toBeEnabled();
expect(field).toBeVisible();
const testData = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit but does this ever change? seems like a constant. const TEST_DATA or const SAMPLE_ADDRESS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want this data to change so used const. If we need more data, we can create another const and/or nest it in different blocks. I preferred having one object with keys rather than having 4 different variables for each key. I went with lowerCamelCase to be consistent with the file.

@gina-yamada gina-yamada force-pushed the yamada/LG-11082-RefactorToUseFullAddressSearch branch from be0a651 to 9b59f32 Compare October 11, 2023 14:18
@gina-yamada gina-yamada merged commit 48708b1 into main Oct 11, 2023
3 checks passed
@gina-yamada gina-yamada deleted the yamada/LG-11082-RefactorToUseFullAddressSearch branch October 11, 2023 14:49
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.

None yet

3 participants