-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add registration dialog #34
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull request overview
This PR adds a registration dialog feature that allows users to register for events through a modal form. The implementation includes new custom hooks to refactor event operations, a reusable Dialog UI component, and an event registration form dialog. Additionally, date formatting logic has been extracted into a utility function to promote code reuse.
Key changes:
- Added Radix UI Dialog component and event registration form dialog with dynamic field rendering
- Refactored event mutations into reusable hooks (useCreateEvent, useUpdateEvent, usePublishEvent)
- Extracted date formatting logic into a shared utility function
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added @radix-ui/react-dialog dependency for dialog functionality |
| apps/web/package.json | Added @radix-ui/react-dialog package reference |
| apps/web/src/styles.css | Adjusted popover background color for better consistency |
| apps/web/src/components/ui/dialog.tsx | New Dialog UI component wrapper for Radix UI Dialog primitive |
| apps/web/src/components/forms/event-registration-form-dialog.tsx | New registration form dialog with dynamic field rendering based on event form structure |
| apps/web/src/lib/hooks/use-create-event.tsx | Extracted event creation logic into reusable hook |
| apps/web/src/lib/hooks/use-update-event.tsx | Extracted event update logic into reusable hook |
| apps/web/src/lib/hooks/use-publish-event.tsx | Extracted event publishing logic into reusable hook |
| apps/web/src/lib/utils.ts | Added formatEventDate utility function for consistent date formatting |
| apps/web/src/routes/events/create.tsx | Refactored to use useCreateEvent hook and pass loading state to form |
| apps/web/src/routes/events/$eventId/edit.tsx | Refactored to use useUpdateEvent hook and pass loading state to form |
| apps/web/src/routes/events/$eventId/index.tsx | Added registration dialog with open state management and integrated form submission placeholder |
| apps/web/src/routes/events/draft.tsx | Simplified event list rendering logic |
| apps/web/src/components/event-card.tsx | Refactored to use formatEventDate utility function |
| apps/web/src/components/forms/modify-event-form.tsx | Added isLoading prop to disable all form inputs during submission |
| apps/shared/src/registrations/schemas.ts | Simplified RegistrationAnswerSchema to only accept string values |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| showCloseButton?: boolean | ||
| }) { | ||
| return ( | ||
| <DialogPortal data-slot="dialog-portal"> |
Copilot
AI
Jan 6, 2026
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.
The DialogPortal component is being passed a data-slot prop at line 58, but React components don't accept arbitrary props unless they're explicitly spread. The data-slot attribute should be placed on the elements rendered by DialogPortal, not on the component itself. This prop will be ignored and may cause confusion. Remove the data-slot prop from this line.
| <DialogPortal data-slot="dialog-portal"> | |
| <DialogPortal> |
| </Button> | ||
| ) : ( | ||
| <Button>Register Now</Button> | ||
| <Button onClick={() => setOpen(!open)} disabled={!isAuthenticated}> |
Copilot
AI
Jan 6, 2026
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.
The onClick handler toggles the open state using !open, but this creates potential race conditions if the button is clicked multiple times rapidly. Since Dialog manages its own state, you should use setOpen(true) to explicitly open the dialog rather than toggling based on current state.
| z.string(), | ||
| z.string().or(z.array(z.string())).optional() | ||
| ); | ||
| export const RegistrationAnswerSchema = z.record(z.string(), z.string()); |
Copilot
AI
Jan 6, 2026
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.
The schema change removes support for array values (z.array(z.string())) from RegistrationAnswerSchema, but the buildRegistrationSchema function in event-registration-form-dialog.tsx doesn't handle multi-select fields. If multi-select fields were previously supported or are planned, this breaking change could cause data loss or errors. If array values are no longer needed, ensure this is intentional and that no existing registrations use array values.
| export const RegistrationAnswerSchema = z.record(z.string(), z.string()); | |
| export const RegistrationAnswerSchema = z.record( | |
| z.string(), | |
| z.union([z.string(), z.array(z.string())]), | |
| ); |
| </Sheet> | ||
| {event.form !== null && ( | ||
| <EventRegistrationFormDialog | ||
| onFormSubmit={() => {}} |
Copilot
AI
Jan 6, 2026
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.
The onFormSubmit handler is set to an empty function that does nothing. This means registrations cannot actually be submitted. The handler should either submit the registration data to a backend API or update this to indicate work is in progress with a TODO comment.
| onFormSubmit={() => {}} | |
| onFormSubmit={(formData) => { | |
| // TODO: Implement event registration submission, e.g. POST formData to a backend API. | |
| console.warn('Event registration submission is not yet implemented.', formData) | |
| }} |
| )} | ||
| </FieldGroup> | ||
| </div> | ||
| <DialogFooter className="justify-start!"> |
Copilot
AI
Jan 6, 2026
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.
There's a typo in the CSS class name: "justify-start!" should be "!justify-start" following Tailwind's important modifier syntax. The exclamation mark should precede the utility class name, not follow it.
| <DialogFooter className="justify-start!"> | |
| <DialogFooter className="!justify-start"> |
| formStructure: Array<CustomField> | ||
| isLoading?: boolean | ||
| isOpen: boolean | ||
| onOpenChange: () => void |
Copilot
AI
Jan 6, 2026
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.
The onOpenChange prop type is defined as () => void, but the Dialog component from Radix UI expects (open: boolean) => void. This type mismatch means the component won't receive the proper boolean value indicating whether the dialog should be open or closed. Update the type to onOpenChange: (open: boolean) => void to match the Dialog API.
| onOpenChange: () => void | |
| onOpenChange: (open: boolean) => void |
| ? z.enum(field.options as [string, ...Array<string>], { | ||
| error: () => ({ message: `${field.label} is required` }), | ||
| }) | ||
| : z.enum(field.options as [string, ...Array<string>]).optional() |
Copilot
AI
Jan 6, 2026
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.
When a select field is optional and no value is selected, the form will submit an empty string '' as the value. However, z.enum().optional() expects either undefined or one of the enum values, not an empty string. This will cause validation errors when submitting optional select fields without a selection. Consider using .or(z.literal('')) or filtering out empty strings before validation.
| : z.enum(field.options as [string, ...Array<string>]).optional() | |
| : z | |
| .enum(field.options as [string, ...Array<string>]) | |
| .or(z.literal('')) | |
| .optional() |
| onFormSubmit={() => {}} | ||
| formStructure={event.form} | ||
| isOpen={open} | ||
| onOpenChange={() => setOpen(!open)} |
Copilot
AI
Jan 6, 2026
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.
The onOpenChange callback uses a toggle pattern (!open) which can lead to state synchronization issues. Since the Dialog component already manages its internal state and calls onOpenChange when it wants to close, you should directly set the desired state value rather than toggling it. Consider using onOpenChange={(isOpen) => setOpen(isOpen)} or simply onOpenChange={setOpen} to match the Dialog's state more reliably.
| onOpenChange={() => setOpen(!open)} | |
| onOpenChange={setOpen} |
| data: { id: eventId, state: EventState.Published }, | ||
| }), | ||
| onSuccess: (event) => { | ||
| void queryClient.invalidateQueries({ queryKey: ['events', eventId] }) |
Copilot
AI
Jan 6, 2026
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.
When publishing an event, the hook only invalidates the specific event query ['events', eventId] but doesn't invalidate the events list queries ['events', { state: 'draft' }] or ['events', { state: 'published' }]. This means the draft events list won't reflect that the event has been removed, and the published events list won't show the newly published event until a manual refresh. Consider adding queryClient.invalidateQueries({ queryKey: ['events'] }) to invalidate all events queries.
| void queryClient.invalidateQueries({ queryKey: ['events', eventId] }) | |
| void queryClient.invalidateQueries({ queryKey: ['events'] }) |
| <form | ||
| id="registration-form" | ||
| onSubmit={(e) => { | ||
| e.preventDefault() | ||
| void form.handleSubmit() | ||
| }} | ||
| > | ||
| <DialogContent> | ||
| <DialogHeader> | ||
| <DialogTitle>Register for {eventTitle}</DialogTitle> | ||
| </DialogHeader> | ||
| <DialogDescription> | ||
| Please fill out the additional details below to secure your spot. | ||
| </DialogDescription> | ||
| <div className="max-h-[65vh] overflow-auto py-5"> | ||
| <FieldGroup> | ||
| {formStructure.length === 0 ? ( | ||
| <div className="text-center py-8"> | ||
| <p className="text-sm text-muted-foreground"> | ||
| No registration fields configured for this event. | ||
| </p> | ||
| </div> | ||
| ) : ( | ||
| <>{formStructure.map((field) => renderField(field))}</> | ||
| )} | ||
| </FieldGroup> | ||
| </div> | ||
| <DialogFooter className="justify-start!"> | ||
| <Button | ||
| type="submit" | ||
| form="registration-form" | ||
| className="w-full md:max-w-fit" | ||
| disabled={isLoading} | ||
| > | ||
| {isLoading ? 'Submitting...' : 'Submit Registration'} | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| </form> |
Copilot
AI
Jan 6, 2026
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.
The form element is placed outside the DialogContent, which may cause issues with form submission and keyboard navigation. In Radix UI Dialog, portal-based content can have accessibility and focus management issues when form elements are structured this way. Consider placing the form element inside the DialogContent or wrapping just the DialogContent with the form to ensure proper behavior.
No description provided.