Conversation
WilliamBirdsall
force-pushed
the
will/LG-14958
branch
from
98b46b0 to
bef05c7
Compare
WilliamBirdsall
requested review from
a team,
KeithNava and
gina-yamada
and removed request for
KeithNava
…s_entry_enabled feature flag usage.
WilliamBirdsall
force-pushed
the
will/LG-14958
branch
from
cbb5ef6 to
743a0c7
Compare
eileen-nava
left a comment
eileen-nava
left a comment
There was a problem hiding this comment.
Approved. I left non-blocking comments. Good work, Will. 👏🏻
| const inPersonLocationPostOfficeSearchForm = inPersonFullAddressEntryEnabled | ||
| ? InPersonLocationFullAddressEntryPostOfficeSearchStep | ||
| : InPersonLocationPostOfficeSearchStep; | ||
| const inPersonLocationPostOfficeSearchForm = InPersonLocationFullAddressEntryPostOfficeSearchStep; |
There was a problem hiding this comment.
I only see the variable inPersonLocationPostOfficeSearchForm used once in this file, on L96 in the creation of locationFormStep. Is there any downside to removing the variable inPersonLocationPostOfficeSearchForm and just using InPersonLocationFullAddressEntryPostOfficeSearchStep on L96?
| end | ||
|
|
||
| context 'when full form address entry is enabled for post office search' do | ||
| context 'when full form address post office search' do |
There was a problem hiding this comment.
Nit: This could probably be condensed into just an it statement.
There was a problem hiding this comment.
Oh like we can remove the wrapping context? Wasn't sure if it was helpful due to the user creation line directly under it, but I guess there aren't any other it blocks that would use that so removing is probably a good idea.
Ill be in here again once the form is simplified to zip. Should add to that ticket to make sure test structure is as simple as possible? Or just simplify it now?
There was a problem hiding this comment.
To me, the context block felt unnecessary because the user will always be using full form address post office search. I would see the value if we had another option enabled, like searching by zip code, but that's not the app's current state.
I also think simplifying this to an it statement felt consistent with the top of the file, which uses it statements for conditions that I'd always expect to be true. For example, in the same way that we expect the app to always allow the user to cancel and start over from the beginning, we expect the app to always allow the user to search by full address. Does that make sense?
The it block I used as an example creates the user variable inside of it, and I generally prefer to create that kind of variable outside of it blocks with a let statement. So, maybe the example I'm using isn't the best, either. 😆 I'm also good with you leaving this code as is! It's up to you. The comment is a non-blocking nit.
There was a problem hiding this comment.
Definitely makes sense! I think ill keep it in there for this PR but it will probably change across the next couple PRs that deal more directly with zip stuff and code removal. I agree with your thinking for sure!
| import SelfieStep from './selfie-step'; | ||
| import DocumentsStep from './documents-step'; | ||
| import InPersonPrepareStep from './in-person-prepare-step'; | ||
| import InPersonLocationPostOfficeSearchStep from './in-person-location-post-office-search-step'; |
There was a problem hiding this comment.
This is the only place that the component is used, can we remove the file at in-person-location-post-office-search-step.tsx?
There was a problem hiding this comment.
There is another ticket for actually removing the old code, so that will be deleted next!
3 participants
🎫 Ticket
Link to the relevant ticket:
LG-14958
🛠 Summary of changes
This ticket evolves removing the in_person_full_address_entry_enabled flag, which should no longer be toggle-able.