Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
zomars
requested review from
a team,
joeauyeung and
sean-brydon
and removed request for
joeauyeung
| const [hashedLinkVisible, setHashedLinkVisible] = useState(!!eventType.hashedLink); | ||
| const [hashedUrl, setHashedUrl] = useState(eventType.hashedLink?.link); | ||
| const [seatsInputVisible, setSeatsInputVisible] = useState(!!eventType.seatsPerTimeSlot); | ||
| const [seatsPerTimeSlot, setSeatsPerTimeSlot] = useState(eventType.seatsPerTimeSlot || 2); |
There was a problem hiding this comment.
When using form, we should not rely on useState as much as possible. We already have a "Form State" we should take advantage of it as much as possible.
| <hr /> | ||
| <Controller | ||
| name="seatsPerTimeSlot" | ||
| name="seatsPerTimeSlotEnabled" |
There was a problem hiding this comment.
Added a pseudo-field that won't be sent to the backend but will allow us to keep track of this state locally.
|
|
||
| const animationRef = useRef(null); | ||
| const seatsEnabled = !!eventType.seatsPerTimeSlot; | ||
| const seatsEnabled = formMethods.getValues("seatsPerTimeSlotEnabled"); |
There was a problem hiding this comment.
As per my comment above. Use the form as much as possible.
| // Enabling seats will disable guests and requiring confimation until fully supported | ||
| if (e) { | ||
| formMethods.setValue("disableGuests", true); | ||
| formMethods.setValue("requiresConfirmation", false); | ||
| formMethods.setValue("seatsPerTimeSlot", 2); | ||
| } else { | ||
| formMethods.setValue("seatsPerTimeSlot", null); | ||
| } |
There was a problem hiding this comment.
Here's the actual logic, we don't need to manipulate external state, just the actual form values.
| /> | ||
| {seatsInputVisible && ( | ||
| <Controller | ||
| name="seatsPerTimeSlot" |
There was a problem hiding this comment.
The Switch (which is a boolean) was manipulating a number value. This makes it right.
| className={classNames( | ||
| props.checked ? "bg-gray-900" : "bg-gray-200 hover:bg-gray-300", | ||
| props.checked ? "bg-gray-900" : "bg-gray-200", | ||
| primitiveProps.disabled ? "cursor-not-allowed" : "hover:bg-gray-300", |
There was a problem hiding this comment.
At least for desktop users, disabled switches will display a "non allowed" cursor. Tried adding opacity as well but it becomes confusing between on-disabled/off-disabled/on-enabled/off-enabled cc @Jaibles any suggestions for mobile users?
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
I don't really like pseudo-fields, at that point you're better off with state again, which Controller under the hood also does. Minor nit.
We could make it an actual field and move this logic to the backend tho. "If input. seatsPerTimeSlotEnabled is false then set the seatsPerTimeSlot to null" |
I'd say no as it's purely FE logic, if I ask myself "What if I update through the API" that doesn't make sense. But asking myself the same question the following logic does come to mind as belonging to the BE. // Enabling seats will disable guests and requiring confirmation until fully supported
if (e) {
formMethods.setValue("disableGuests", true);
formMethods.setValue("requiresConfirmation", false);
formMethods.setValue("seatsPerTimeSlot", 2);
} else {
formMethods.setValue("seatsPerTimeSlot", null);
}Maybe not as a side-effect but as a guards; e.g. "Requiring confirmation is not possible during events with multiple seats." or "Unable to disable guests as this event has multiple seats". |
emrysal
added
♻️ autoupdate
2 participants
What does this PR do?
Grabacion.de.pantalla.2022-09-28.a.la.s.12.17.14.mov
Environment: Staging(main branch) / Production
Type of change
How should this be tested?