Skip to content

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

Merged
gina-yamada merged 8 commits intomainfrom
yamada/LG-11082-RefactorToUseFullAddressSearch
Oct 11, 2023
Merged

LG-11082 Refactor Post Office Search to use FullAddressSearch#1174
gina-yamada merged 8 commits intomainfrom
yamada/LG-11082-RefactorToUseFullAddressSearch

Conversation

@gina-yamada
Copy link
Copy Markdown
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

Comment thread assets/js/form_helper.tsx
@gina-yamada gina-yamada marked this pull request as ready for review October 5, 2023 18:59
Comment thread spec/js/post_office_search_spec.ts Outdated
@allis-green
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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.

3 participants