Skip to content

LG-8330: Allow user to select USPS location when search box is empty#7760

Merged
eileen-nava merged 5 commits intomainfrom
em/8330-cant-select-ipp
Feb 3, 2023
Merged

LG-8330: Allow user to select USPS location when search box is empty#7760
eileen-nava merged 5 commits intomainfrom
em/8330-cant-select-ipp

Conversation

@eileen-nava
Copy link
Contributor

@eileen-nava eileen-nava commented Feb 2, 2023

🎫 Ticket

8330: User cannot select an IPP location if search box is empty

🛠 Summary of changes

  • User can now select a USPS location when the address search box is empty
  • User can now proceed past the PO Search page when the address search box is empty

📜 Testing Plan

Happy Path

  • Navigate to the PO Search page and search for a valid address
  • After USPS locations load, delete the text from the search box
  • Click the "select" button for a USPS location

Edge Cases
Run through the happy path, but then....

  • Navigate back to the PO Search Page. Confirm you can select a different USPS location.
    Run through the happy path, but then....
  • Navigate back to the PO Search Page. Confirm you can search for a different address and select a USPS location.

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Feature In Action:
SelectLocationWithEmptySearchBox.mov

@eileen-nava eileen-nava changed the title Allow user to LG-8330: Allow user to select USPS location when search box is empty Feb 2, 2023
@eileen-nava eileen-nava requested review from a team and jess-fortier February 2, 2023 20:05
onChange={onTextInputChange}
label={t('in_person_proofing.body.location.po_search.address_search_label')}
hint={t('in_person_proofing.body.location.po_search.address_search_hint')}
disabled={disabled}
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting... how does this fix the issue? Does the FormSteps stuff ignore disabled fields even after invalidation?

Copy link
Contributor

Choose a reason for hiding this comment

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

@NavaTim I'm just curious — how does this work/fix the bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

@allthesignals: I think this answers what you're asking, but please say so if I missed your q. @eileen-nava filled me in on the inner workings here. The form steps component is just a standard form element, so when you pass an attribute of 'disabled' to that form field, it does not validate that field. TIL: MDN Disabled Overview

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, great! I assumed FormSteps was doing validation... sounds like it's being done natively!

Copy link
Contributor

Choose a reason for hiding this comment

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

@allthesignals The ValidatedFieldElement hooks into the invalid event, which doesn't trigger when the element is disabled.

Relevant spec sections:

valid = element.checkValidity()
Returns true if the element's value has no validity problems; false otherwise. Fires an invalid event at the element in the latter case.

Source

Note that the invalid event in the above source is mentioned as being triggered by the constraint validation API.

Constraint validation: If an element is disabled, it is barred from constraint validation.

Source

A submittable element is a candidate for constraint validation except when a condition has barred the element from constraint validation. (For example, an element is barred from constraint validation if it is an object element.)

Source

Copy link
Contributor

Choose a reason for hiding this comment

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

Fascinating. Thank you Tim!

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.

Overall PR looks good and like what we discussed, but would be good to add a test here to prevent regression.

@eileen-nava
Copy link
Contributor Author

Overall PR looks good and like what we discussed, but would be good to add a test here to prevent regression.

Good point, I added a test.

Comment on lines +396 to +398
await userEvent.clear(
await findByLabelText('in_person_proofing.body.location.po_search.address_search_label'),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably would move this and related prereq steps to the beforeEach, but it's not very important at the moment because we only have one it in this context.

@eileen-nava eileen-nava merged commit a0004a7 into main Feb 3, 2023
@eileen-nava eileen-nava deleted the em/8330-cant-select-ipp branch February 3, 2023 18:26
@aduth aduth mentioned this pull request Feb 9, 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.

4 participants