Skip to content

Fix form record picker field #11817

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix form record picker field #11817

wants to merge 3 commits into from

Conversation

thomtrp
Copy link
Contributor

@thomtrp thomtrp commented Apr 30, 2025

  • enrich response so the record is available in the step output. Today this is available in the schema but only the id is set
  • make the full record picker clickable instead of the arrow only
Capture d’écran 2025-04-30 à 16 08 04

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR enhances form record picker functionality by making the entire picker clickable and enriching workflow step outputs with complete record data instead of just IDs.

  • Added enrichFormStepResponse method in workflow-version-step.workspace-service.ts to fetch full record data for form submissions
  • Updated FormSingleRecordPicker styling with hover states and cursor pointer, changing dropdown placement to 'bottom-start'
  • Modified Dropdown component to support configurable widths via new clickableComponentWidth prop
  • Added proper event propagation handling in FormSingleRecordFieldChip for remove actions
  • Added 'INVALID' exception code to handle form validation errors

5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

github-actions bot commented Apr 30, 2025

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:21639

This environment will automatically shut down when the PR is closed or after 5 hours.

@thomtrp thomtrp force-pushed the tt-fix-form-select-record branch from 65a0f1c to 3927495 Compare April 30, 2025 15:21
Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! The forms will look better after we merge it.
Let's discuss a few elements before merging.

closeDropdown();

const handleUnlinkVariable = (event?: React.MouseEvent<HTMLDivElement>) => {
event?.stopPropagation();
Copy link
Contributor

Choose a reason for hiding this comment

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

For anyone reading this PR, the stopPropagation prevents the dropdown to open when clicking on the "Delete" icon of a VariableChipStandalone. See the video below.

CleanShot.2025-04-30.at.18.02.35.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel there might be a less "hacky" solution to this. We might ask Lucas.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's create stories for this component. We did a great job writing stories for most of the Form components, and this component shouldn't be an exception.

`;

export type DropdownProps = {
className?: string;
clickableComponent?: ReactNode;
clickableComponentWidth?: Width;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you took the time to find the best solution to this problem, but I wonder if we didn't find an API design flaw in the Dropdown component. We can provide a clickableComponent, but won't the width of the clickable area expand to the bounds of the clickableComponent? This looks strange to me, and maybe we should discuss it.

);

const record = await repository.findOne({
where: { id: response[key].id },
Copy link
Contributor

@Devessier Devessier Apr 30, 2025

Choose a reason for hiding this comment

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

As discussed in private, an error is thrown here after having clicked on the option "No notes" because we send a string that's not a UUID and that throws here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants