-
Notifications
You must be signed in to change notification settings - Fork 24
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Autofill applications for signed in users #1111
Conversation
Deploy preview for clever-edison-cd22c1 ready! Built with commit e7e4802 https://deploy-preview-1111--clever-edison-cd22c1.netlify.app |
@kathyccheng Here's the rundown of all the stuff that will get changed or deleted when the application is loaded up:
And then various other sections being deleted/reset:
Let me know if that makes sense to you |
@slowbot Here's that issue I mentioned regarding the final page link: Also, on the first form step when somebody's already logged in, I wasn't sure what to do there so there's just a bit of white space. Let me know if you have any thoughts. |
@jaredcwhite all the things you removed makes sense to me! |
@slowbot will let you take a look at whitespace. for the weird No thanks, I'm Done thing, were you able to figure it out @jaredcwhite? |
I still need guidance on the "No thanks" language. |
Can we do "I'm done" or just remove it? @jaredcwhite @slowbot |
I'm happy to change it to "I'm done" but I don't know how to change the translations. |
Like we don't have the translations, or you can't find it in our translation file? @jaredcwhite |
Oh, I mean I don't know what the new translations should be. |
Gotcha! That's no problem, I'll add it to the next wave of translations with our contractors! |
Sounds good. And I just realized I have some strings which need translating on the new autofill page(s), so let me move those to the English JSON which I'll send to you for reference. |
@kathyccheng Here's what I've got so far: {
"application": {
"autofill": {
"saveTime": "Save time by using the details from your last application",
"prefillYourApplication": "We'll simply pre-fill your application with the following details, and you can make updates as you go.",
"start": "Start with these details",
"reset": "Reset and start fresh"
},
"household": {
"addMembers": {
"doubleCheck": "Please double-check the information for each household member.",
},
},
"review": {
"confirmation": {
"imdone": "I'm done."
}
}
}
} |
Perfect, thanks! |
@@ -45,6 +50,20 @@ export default () => { | |||
const emailPresent: string = watch("applicant.emailAddress") | |||
const noEmail: boolean = watch("applicant.noEmail") | |||
const clientLoaded = OnClientSide() | |||
if (!autofilled && clientLoaded && application.autofilled) setAutofilled(true) | |||
|
|||
const AutofillIcon = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Non-blocking) I find this name to be a lil confusing, maybe something like DisabledIcon or LockIcon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense.
{t("account.createAccount")} | ||
</Button> | ||
</div> | ||
{initialStateLoaded && !profile && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious what this change is for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should only show an option to create a new account if the logged-in state has loaded and the user isn't logged in.
</> | ||
)} | ||
</div> | ||
{!hidePreferences && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about the need behind this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't show any preference information on the autofill preview since we're blanking them all out.
@@ -28,4 +29,9 @@ describe("<DOBField>", () => { | |||
expect(getByLabelText("Day")).not.toBeNull() | |||
expect(getByLabelText("Year")).not.toBeNull() | |||
}) | |||
|
|||
it("can toggle all internal fields to be disabled", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎈 🎊
const { member, type } = props | ||
const router = useRouter() | ||
let editMode = props.editMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use a default prop value here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man, this must have been some weird debugging thing. Bad pattern! 😬 Will fix…
OK I figured out the infinite render issue and got that fixed…but I'm not super happy with the code. 50 LoC just to check an endpoint for data and either redirect or provide a form handler feels excessive. Open to any feedback on how to improve. |
const { member, type } = props | ||
const router = useRouter() | ||
const editMode = props.editMode !== false // undefined should default to true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was hoping to use a default prop of false here but this is def non-blocking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the weirdness is that the default state should actually be true, so only if editMode prop is present and set specifically to false should it be set false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaredcwhite probably it's not in this scope, but when I filled out an application, I saw no primary applicant data:
Same situation with "Do you work in question.
} | ||
|
||
addDefaults() { | ||
;["id", "createdAt", "updatedAt", "deletedAt", "listing", "submissionDate"].forEach((key) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
;
<- I suppose it's not necessary :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier is adding it…must have thought there could be a JS error otherwise.
unsetIdentifiers(this.application.applicant) | ||
unsetIdentifiers(this.application.mailingAddress) | ||
|
||
if (this.application.alternateAddress) unsetIdentifiers(this.application.alternateAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are sure that it works? I think alternateAddress
is loaded from the blackApplication.ts and it's an object, so it always has values (?) NOT SURE - just to check :)
The similar situation below with alternateContact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything's coming directly from the backend in an autofill scenario (which is only when this code gets run), so I'm trying to strip out or change data to fit that flow.
sites/public/lib/appAutofill.ts
Outdated
street: "", | ||
street2: "", | ||
city: "", | ||
state: "", | ||
zipCode: "", | ||
county: "", | ||
latitude: null, | ||
longitude: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think could be better to load defaults from the blankApplication.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I could load that in and copy the object over.
<Button | ||
styleType={AppearanceStyleType.primary} | ||
onClick={() => { | ||
useDetails = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be processed by React and useState
? I mean useDetails
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it's not a state change, it's just a runtime flag that lets the submit handler know which action to take. I specifically wouldn't want to trigger any re-render.
@dominikx96 Jesse saw the same issue as well so I will investigate further. Thanks for the feedback! |
@slowbot I think I identified and fixed a low-level bug regarding object references that was causing some data loss…the bug had been present before the autofill PR, but never triggered because we'd never needed to perform a wholesale swap of application data mid-flow before. Can you do one more QA pass? |
Looking good testing on Netlify preview, got the thumbs up from @slowbot…so plan to merge after fixing the conflict. |
Getting some Cypress timeouts but I'm assuming those are spurious. 😬 LGTM |
Resolves #1040 #1041
There's also an old duplicate here: #249
This also encompasses some fixes to the application form flow I noticed when signed in.