fix: UI mismatch with form state#10651
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
@MehulZR is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
|
While updating the PR, I noticed that it is possible to book a event with length of 0. This is possible when saving the form from a tab other than the Event Setup.
We can solve it in two ways.
If you would like the 2nd approach too. What Error message do you want to be shown?
|
|
|
hariombalhara
left a comment
There was a problem hiding this comment.
Let's address merge conflicts and unrelated changes first.
Udit-takkar
left a comment
There was a problem hiding this comment.
@MehulZR do not push yarn.lock unless you change package.json.
Use this command to undo the changes in yarn.lock
|
This PR is being marked as stale due to inactivity. |
|
@MehulZR can you fix the conflicts in this PR? |
| eventType={eventType} | ||
| seatsEnabled={seatsEnabled} | ||
| metadata={eventType.metadata} | ||
| metadata={formMethods.getValues("metadata")} |
There was a problem hiding this comment.
I don't like the idea of sometimes getting the value from eventType and sometimes from formMethods. It makes the code complex for little gains.
Can we not just use formMethods everywhere. We could maybe do eventType = formMethods.getValues() and then we can keep on accessing values from eventType.
There was a problem hiding this comment.
Going with formMethods only is what I will prefer too, will look into it.
There was a problem hiding this comment.
There are some cases when we need the original eventType prop alongside formMethods values.
For ex. having both the count of webhooks and other form values in the same component.
what we can do is instead of calling formMethods.getValues("some_property_name") everywhere, we just do formValues = formMethods.getValues() saving the execution calls and cleaning up things.
but formMethods.getValues isn't reactive (value change doesn't cause re-render & it doesn't subscribe to input changes), so for some cases we will still be using formMethods.watch().
There was a problem hiding this comment.
I have tested a bit with using the formValues strategy, as expected it is not reliable. We should use formMethods.getValues("some_prop")
hariombalhara
left a comment
There was a problem hiding this comment.
@MehulZR I resolved the conflicts and left some more comments.
|
@MehulZR just a heads up there's more conflicts |
joeauyeung
left a comment
There was a problem hiding this comment.
Double checked everything and tested some fields. I fixed the noShowFeeEnabled since it was returning true even if Stripe was disabled.
I did notice one thing that's consistent in main as well but for managed event types when changing from a set availability and back to "member's default availability" the option doesn't change.
I'm ok approving this PR since that bug wasn't caused by the changes made and it's such a huge QOL improvement.
Changes have been incorporated
emrysal
left a comment
There was a problem hiding this comment.
Fixed merge conflicts, approved
What does this PR do?
Fixes #10647 (issue)
Fixes #10390 (issue)video.mp4
Type of change
Mandatory Tasks