Skip to content

LG-10330 No PO Results Component#9145

Merged
gina-yamada merged 21 commits intomainfrom
yamada/LG-10330-PoNoResultsComponent
Sep 7, 2023
Merged

LG-10330 No PO Results Component#9145
gina-yamada merged 21 commits intomainfrom
yamada/LG-10330-PoNoResultsComponent

Conversation

@gina-yamada
Copy link
Contributor

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

🎫 Ticket

LG-10330 Make PO Search's no results view configurable -- This PR is just to make PO Search No Results View (the default view) and use it in InPersonLocations.

🛠 Summary of changes

  • Added new svg
  • Added alt text translations for new image
  • Created new NoInPersonLocationsDisplay component that contains the new svg image and design (The stuff below the Search button only), see FIGMA Mocks
  • Added component to export list for address-search
  • Changed version of @18f/identity-address-search from 2.0.0 to 2.1.0 (I believe next step is to publish this new version so that I can use this component via import from 18f rather than file path)

📜 Testing Plan

  • Step 1: View screenshots below- before and after.
  • Step 2: Start repo locally. Run through IPP to PO Search. (You will have to temporarily delete objects in postOffices array in request_facilities_response.json to see this view. Be sure to test when in_person_full_address_entry_enabled is set to true and set to false)

👀 Screenshots

Before
Screenshot 2023-09-06 at 4 03 42 PM

After
Screenshot 2023-09-06 at 9 34 22 AM

Screenshots to show alt text applied translations

Screenshot 2023-09-06 at 3 20 09 PM

Screenshot 2023-09-06 at 3 18 45 PM

Screenshot 2023-09-06 at 3 19 21 PM

@gina-yamada gina-yamada marked this pull request as ready for review September 5, 2023 20:18
@gina-yamada gina-yamada requested review from a team and carmenrosalop and removed request for a team September 6, 2023 15:07
Copy link
Contributor

@NavaTim NavaTim left a comment

Choose a reason for hiding this comment

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

If we want to merge this ahead of your other PR, then let's hardcode the PO search to use this component & make the other PR about making the component configurable. Make sure to re-verify that the screenshots are accurate after making the change.

@gina-yamada
Copy link
Contributor Author

gina-yamada commented Sep 6, 2023

If we want to merge this ahead of your other PR, then let's hardcode the PO search to use this component & make the other PR about making the component configurable. Make sure to re-verify that the screenshots are accurate after making the change.

@NavaTim Resolved with commit 1196fde - Screenshots include time- I tested just before I pushed up this commit.

Screenshot 2023-09-06 at 3 50 37 PM

Screenshot 2023-09-06 at 3 52 07 PM

Copy link
Contributor

@NavaTim NavaTim left a comment

Choose a reason for hiding this comment

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

Spacing between the button and result text looks different from the Figma, but if that's within an acceptable margin then I think this otherwise looks good.

@gina-yamada
Copy link
Contributor Author

gina-yamada commented Sep 6, 2023

@NavaTim Thanks for your review/approval. I just pulled in main and ran through one more manual test.

When in_person_full_address_entry_enabled is set to false I am getting an API error so I am not able to hit my new view. It looks like that is expected behavior at this time as you just merged in PR 9154.

Can you confirm this banner is the expected behavior when in_person_full_address_entry_enabled is set to false?

Screenshot 2023-09-06 at 4 31 28 PM

@NavaTim
Copy link
Contributor

NavaTim commented Sep 6, 2023

When in_person_full_address_entry_enabled is set to false I am getting an API error so I am not able to hit my new view. It looks like that is expected behavior at this time as you just merged in #9154.

Yes, I merged #9154 into main earlier today which removes the geocoding endpoint altogether. Going forward we will only use the full address search. in_person_full_address_entry_enabled will also now default to true.

Comment on lines +8 to +9
className="grid-col-2"
style={{ marginTop: '30px' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Can we use design system utility class instead of the inline style?
  • From what I can see in the design, it looks like this should be 40px gap, not 30px ? (Asking also because 30px is not a multiple of the 8px design system base size)
Suggested change
className="grid-col-2"
style={{ marginTop: '30px' }}
className="grid-col-2 margin-top-5"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved with commit 18469c9 - I also adjusted the grid-gap to once after reading this. Tim was right- grid-gap should have been 8 (1) not 16 (2).

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we're using a subpixel value for height? The change from 65 to 64.6

Copy link
Contributor

@NavaTim NavaTim Sep 7, 2023

Choose a reason for hiding this comment

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

@gina-yamada Subpixel values have historically been prone to rendering differences and bugs, so I recommend sticking with whole pixel values. I encountered this issue in Firefox previously when dynamically loading slices of a large image for a side-scrolling parallax feature.

Copy link
Contributor Author

@gina-yamada gina-yamada Sep 7, 2023

Choose a reason for hiding this comment

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

I will adjust the value in my next PR back to 65. I modified it to the value in FIGMA

@gina-yamada gina-yamada merged commit f4a32ae into main Sep 7, 2023
@gina-yamada gina-yamada deleted the yamada/LG-10330-PoNoResultsComponent branch September 7, 2023 15:04
@aduth aduth mentioned this pull request Sep 11, 2023
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.

6 participants