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: fixes multiple issues in routing form #4676

Merged
merged 9 commits into from
Oct 6, 2022

Conversation

G3root
Copy link
Contributor

@G3root G3root commented Sep 23, 2022

What does this PR do?

this PR fixes multiple issues in routing forms. issues fixed:

  • revalidate existing data after adding a form.
  • revalidate existing data after deleting a form and update optimistically.
  • add correct translation key in new form modal description.

issues:
https://www.loom.com/share/f9bce957b58b441ea1330f94e0ad801c
https://www.loom.com/share/2ebd792efeb2448983168f08bfa0db15

after:
https://www.loom.com/share/a827a228b3e84681815a2d5a9b2ce549

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@vercel
Copy link

vercel bot commented Sep 23, 2022

@G3root is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@hariombalhara
Copy link
Member

hariombalhara commented Sep 23, 2022

Thanks @G3root for the untranslated content fix and the PR.

This PR seems to address 2 issues

  1. Untranslated content which is working perfectly fine with your fix.
  2. Forms list not updated with the just created Form. This particular issue doesn't replicate on local. See this loom https://www.loom.com/share/cab4b60caa1f40b594ecc6e40bef1837 for main branch on my local.

But I do see that issue 2 does occur on production but only when a Form is created for the first time. On, consecutive attempts forms list update just fine. See loom https://www.loom.com/share/94d7a3b051194dda88d79f89cc633d70

Forms list updates automatically because the page is SSR and whenever someone goes to the page a .json request is sent by Next.js automatically that updates the forms list page. But this behaviour doesn't seem to trigger for the very first Form creation. We need to figure out why, may be you can help.

Theoretically, your fix of switching to tRPC(with SSR'd data as initial data) would work, but I don't want to involve tRPC on SSR pages which should already have the data we are fetching through tRPC

@hariombalhara
Copy link
Member

If you can open a PR for untranslated content, that can be merged quickly

@G3root
Copy link
Contributor Author

G3root commented Sep 26, 2022

Theoretically, your fix of switching to tRPC(with SSR'd data as initial data) would work, but I don't want to involve tRPC on SSR pages which should already have the data we are fetching through tRPC

yeah you are right, involving tRPC here is might not be the best. the reason i went on with this approach is because, this path viewer.app_routing_forms.forms is revalidated every time a form is created or deleted even though tRPC is not involved.

another advantage involving tRPC is that we can make use of optimistic-updates. what's your thought's on that ?

@hariombalhara
Copy link
Member

hariombalhara commented Sep 26, 2022

this path viewer.app_routing_forms.forms is revalidated every time a form is created or deleted even though tRPC is not involved.

Yeah, that's dead code. It should not have been there.

another advantage involving tRPC is that we can make use of optimistic-updates.

That's a valid point. We are not using optimistic updates in general in the app. But I do think they would provide good UX.

I will take a look at this PR again for the Optimistic updates approach.

@hariombalhara hariombalhara added ♻️ autoupdate tells kodiak to keep this branch up-to-date automerge labels Oct 6, 2022
@hariombalhara
Copy link
Member

@G3root Thanks for the PR. It should be merged soon now.

@kodiakhq kodiakhq bot merged commit ef611e2 into calcom:main Oct 6, 2022
@G3root G3root deleted the fix-routing-form branch October 6, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge ♻️ autoupdate tells kodiak to keep this branch up-to-date
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants