Skip to content
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

[Fix] Fix View Permit Holder API Hookup integration bugs #56

Merged

Conversation

OustanDing
Copy link
Member

@OustanDing OustanDing commented Sep 1, 2021

Fixed permit holder info not updating following edit, fixed routing, ……small improvements

Notion ticket link

Ticket

Implementation description

  • Fixed user information card and modal not being automatically updated following an edit (mutation API call)
  • Fixed Permit Holders page tab not being selected when in View Permit Holder page
  • Various code style improvements
  • Removed SuccessfulEditAlert component as it was unnecessary (just use useToast with status='success' instead)
  • Replaced date of birth input with DatePicker component in EditUserInformationModal component

Notes

Checklist

  • My PR name is descriptive, is in imperative tense and starts with one of the following: [Feature],[Improvement] or [Fix],
  • I have run the appropriate linter(s)
  • I have requested a review from the RCD team on GitHub, or specific people who are associated with this ticket

Copy link
Contributor

@angeladietz angeladietz left a comment

Choose a reason for hiding this comment

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

Looks great! just a few very minor comments but other than that it looks good to go 🚀

tools/components/internal/layout.ts Outdated Show resolved Hide resolved
value={dateOfBirth}
onChange={event => setDateOfBirth(event.target.value)}
/>
<DatePicker width="100%" value={dateOfBirth} onDateChange={setDateOfBirth} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super sure how I feel about using this DatePicker component here. The component doesn't allow for changing the year, only the month, and since birthdays will probably be a long time ago it would take a lot of scrolling back through the months to get to the right year. Because of this, users would probably just end up fully typing the date anyways. Is there anyway we could add editing just the year in the date picker instead of only the month?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a really good point. I just tested it as well, and it's not possible to enter the date manually as the component doesn't take in input for some reason. The old implementation was working fine and easily allowed manually entering the date, so I'll revert to the old version for now. We will probably style it manually in the near future to follow our design system.

We could also add in year editing through this example https://react-day-picker.js.org/examples/elements-year-navigation/ but that would require a new design as well. Also, from a UX standpoint, I don't remember the last time I selected my DOB on a website through a date picker - I just used the date input so I feel like manually entering the date would be the main input method here.

Will bring this up with product team too to get their thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverting back to the HTML date picker input for now, which we will likely stick with. Any changes will be addressed in a future PR.

Copy link
Contributor

@chrischan325 chrischan325 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@angeladietz angeladietz left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Just found a small typo (unrelated to your changes but it'd be great to fix it here quick).

pages/verify-identity.tsx Show resolved Hide resolved
@OustanDing OustanDing merged commit 6e33485 into staging Sep 1, 2021
@OustanDing OustanDing deleted the od/bugfix/fix-view-permit-holder-integration-bugs branch September 1, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants