-
Notifications
You must be signed in to change notification settings - Fork 86
Change request/ID verification flow to use separate pages instead of a modal #7238
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Greptile SummaryConverted the privacy request flow from a modal-based approach to dedicated full-page routes ( Key Changes:
Minor Style Issues:
Confidence Score: 4/5
Important Files Changed
|
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.
13 files reviewed, 2 comments
| }} | ||
| data-testid={dataTestId} | ||
| > | ||
| <div style={{ width: "100%", maxWidth, padding: "48px 24px" }}> |
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.
style: uses div instead of semantic components - per coding standards, prefer Ant Design's Flex component or other semantic elements
| <div style={{ width: "100%", maxWidth, padding: "48px 24px" }}> | |
| <Flex style={{ width: "100%", maxWidth, padding: "48px 24px" }}> |
Context Used: Rule from dashboard - Avoid using div elements when possible. Use semantic HTML elements or component library alternativ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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.
Not as worried about semantic dom here, but some of the styles could use tailwind instead of being inline
| <div | ||
| style={{ | ||
| backgroundColor: "white", | ||
| padding: "48px", | ||
| width: "100%", | ||
| borderRadius: "4px", | ||
| boxShadow: | ||
| "0 1px 3px 0 rgba(0, 0, 0, 0.1), 0 1px 2px 0 rgba(0, 0, 0, 0.06)", | ||
| }} |
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.
style: uses div for form container - prefer semantic or Flex component
| <div | |
| style={{ | |
| backgroundColor: "white", | |
| padding: "48px", | |
| width: "100%", | |
| borderRadius: "4px", | |
| boxShadow: | |
| "0 1px 3px 0 rgba(0, 0, 0, 0.1), 0 1px 2px 0 rgba(0, 0, 0, 0.06)", | |
| }} | |
| <Flex | |
| style={{ | |
| backgroundColor: "white", | |
| padding: "48px", | |
| width: "100%", | |
| borderRadius: "4px", | |
| boxShadow: | |
| "0 1px 3px 0 rgba(0, 0, 0, 0.1), 0 1px 2px 0 rgba(0, 0, 0, 0.06)", | |
| }} | |
| > |
Context Used: Rule from dashboard - Avoid using div elements when possible. Use semantic HTML elements or component library alternativ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
speaker-ender
left a comment
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.
Some minor nits and questions but overall looks good.
| }} | ||
| data-testid={dataTestId} | ||
| > | ||
| <div style={{ width: "100%", maxWidth, padding: "48px 24px" }}> |
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.
Not as worried about semantic dom here, but some of the styles could use tailwind instead of being inline
| iconPath={action.icon_path} | ||
| description={action.description} | ||
| onOpen={onPrivacyModalOpen} | ||
| onOpen={handlePrivacyRequestOpen} |
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.
nit: should we change the onOpen prop to onClick to match the native ant card component since it's not "opening" something?
| if (onSuccessWithoutVerification) { | ||
| onSuccessWithoutVerification(); | ||
| } else { | ||
| onClose(); |
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.
nit: can we rename onClose since we aren't closing a modal?
| }, [getIdVerificationConfigQuery]); | ||
|
|
||
| if (Number.isNaN(parsedActionIndex)) { | ||
| return 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.
Should we re-direct or throw some kind of error message if the route isn't valid instead of displaying nothing?
| const action = config.actions[parsedActionIndex] as | ||
| | PrivacyRequestOption | ||
| | undefined; |
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.
Is there a reason to type cast here?
| params: Promise<{ actionIndex: string }>; | ||
| searchParams: NextSearchParams; | ||
| }) => { | ||
| const { actionIndex } = await params; |
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 like the idea of having a dynamic route for the different actions.
Are we able to/do you think it makes sense to use the action title or some other identifier instead of an index?
Using the index is a little less stable. If someone re-organizes the config, the links to actions would be mixed up.
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 suggestion, I've updated to use the action key instead of the index.
| title, | ||
| }: PrivacyRequestLayoutProps) => { | ||
| return ( | ||
| <AuthFormLayout |
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.
Is this component doing anything other than passing some defaults to the AuthFormLayout component? If so, why not just use the component directly?
Ticket ENG-2283
Description Of Changes
Converted the privacy request flow from a modal-based approach to full-page routes. The form, verification, and success states now use dedicated Next.js pages with a consistent layout. Refactored ExternalAuthLayout to use a shared AuthFormLayout component to reduce code duplication.
Code Changes
ExternalAuthLayoutto useAuthFormLayoutinstead of duplicating layout codeSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works