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

feat(routing-forms): replace FormEdit component with FormBuilder from event-types #19008

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

oliverqx
Copy link

@oliverqx oliverqx commented Jan 29, 2025

/claim #18987

What does this PR do?

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

visit routing-forms, edit or create new one.

Copy link

vercel bot commented Jan 29, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cal-com-ui-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 29, 2025 11:59pm

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@graphite-app graphite-app bot requested a review from a team January 29, 2025 22:32
@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Jan 29, 2025
Copy link

vercel bot commented Jan 29, 2025

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

A member of the Team first needs to authorize it.

@github-actions github-actions bot added event-types area: event types, event-types routing-forms area: routing forms, routing, forms ✨ feature New feature or request 💎 Bounty A bounty on Algora.io 🧹 Improvements Improvements to existing features. Mostly UX/UI labels Jan 29, 2025
@dosubot dosubot bot added this to the v5.0 milestone Jan 29, 2025
@oliverqx oliverqx marked this pull request as draft January 29, 2025 22:33
Copy link

graphite-app bot commented Jan 29, 2025

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (01/29/25)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (01/29/25)

1 label was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR • (01/31/25)

1 reviewer was added to this PR based on Keith Williams's automation.

Page={({ hookForm, form }) => <FormEdit appUrl={appUrl} hookForm={hookForm} form={form} />}
Page={({ hookForm, form }) => (
<>
{/* <FormEdit appUrl={appUrl} hookForm={hookForm} form={form} /> */}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{/* <FormEdit appUrl={appUrl} hookForm={hookForm} form={form} /> */}

Copy link
Contributor

@Praashh Praashh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add test cases and a loom video for this feature?

@oliverqx
Copy link
Author

oliverqx commented Jan 30, 2025

just recorded the loom video

edit: loom video outdated

1. remove filtering by hard coded values in favour of field.systemOnly
2. return options by value instead of label. return array is used to locate fields, and the test id generation uses values, not label. Included comments to explain my reasoning
…r's dynamic

function used to test the previous dynamic, where an accordion was rendered to create and edit fields. Now its a dialog, and data-testids have changed.
@graphite-app graphite-app bot requested a review from a team January 31, 2025 14:54
@oliverqx oliverqx marked this pull request as draft January 31, 2025 16:20
@oliverqx
Copy link
Author

oliverqx commented Feb 1, 2025

Would it be possible to increase the bounty amount?

I’ve been working on adding support for Form Builder fields to Routing Forms widgets, but I just realized that the bounty involves consolidating everything into a single set of custom components.

This requires a deep understanding of multiple areas of the codebase—specifically, Form Builder, Routing Forms, and react-awesome-query-builder’s flow—to ensure there are no unintended side effects.

Additionally, since the test selectors and form dynamics differ significantly between Routing Forms and Form Builder, I’ll need to update all related playwright tests in Routing Forms (editing the form and rendering of the form).

I’ve already put in about 7 hours, and given the complexity, I’d really appreciate a bounty adjustment

cc: @PeerRich @Praashh @hariombalhara

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 Bounty claim 💎 Bounty A bounty on Algora.io community Created by Linear-GitHub Sync event-types area: event types, event-types ✨ feature New feature or request 🧹 Improvements Improvements to existing features. Mostly UX/UI routing-forms area: routing forms, routing, forms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-5097] add the same "booking questions" to routing forms
3 participants