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

Add available variables dropdown #7964

Merged
merged 9 commits into from
Oct 23, 2024
Merged

Conversation

thomtrp
Copy link
Contributor

@thomtrp thomtrp commented Oct 22, 2024

  • Add variable dropdown
  • Insert variables on click
  • Save variable as {{stepName.object.myVar}} and display only myVar
Enregistrement.de.l.ecran.2024-10-22.a.17.30.11.mov

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 pull request introduces a new variable dropdown feature for the workflow editor, focusing on enhancing the user experience when working with variables. Here are the key changes:

  • Implemented SearchVariablesDropdown component for selecting variables in workflows
  • Added VariableTagInput component, combining a Tiptap-based text editor with variable insertion functionality
  • Created utility functions for parsing and initializing editor content with variable tags
  • Introduced unit tests for initializeEditorContent and parseEditorContent functions
  • Updated WorkflowEditActionFormSendEmail to use the new VariableTagInput for email and subject fields

14 file(s) reviewed, 22 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

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

Really cool😍 LGTM

@thomtrp thomtrp enabled auto-merge (squash) October 22, 2024 16:51
@thomtrp thomtrp disabled auto-merge October 23, 2024 07:49
@thomtrp thomtrp force-pushed the tt-add-available-variables-dropdown branch from 7cb545d to 81d75ab Compare October 23, 2024 07:50
@twentyhq twentyhq deleted a comment from greptile-apps bot Oct 23, 2024
@twentyhq twentyhq deleted a comment from greptile-apps bot Oct 23, 2024
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.

Awesome work, @thomtrp! That's so nice to see the variable editor in work. You rock!

I wrote a few comments. Some nitpicks not really important, and some comments about Tiptap's use. I'm happy to discuss them with you. Thank you!

@thomtrp thomtrp force-pushed the tt-add-available-variables-dropdown branch from 63698df to 7dec88c Compare October 23, 2024 10:04
@Devessier Devessier self-requested a review October 23, 2024 13:56
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.

Your updates are awesome. I left a few comments 😄

const parsedContent = parseEditorContent(jsonContent);
onChange?.(parsedContent);
}, 500);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to flush the debouncer to be sure to persist the changes when the right drawer is closed. If the user closes the drawer before the timer ends, the HTTP call will never be made.

I did it that way somewhere else in the application:

  const saveAction = useDebouncedCallback(/* ... */);

  useEffect(() => {
    return () => {
      saveAction.flush();
    };
  }, [saveAction]);

Comment on lines 52 to 63
// @ts-expect-error - addCommands is missing from the NodeConfig type
addCommands: () => ({
insertVariableTag:
(variable: string) =>
({ commands }: { commands: Partial<RawCommands> }) => {
return commands.insertContent?.({
type: 'variableTag',
attrs: { variable },
});
},
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can make TypeScript happy with the following code. It seems to work still. Does it make sense?

Suggested change
// @ts-expect-error - addCommands is missing from the NodeConfig type
addCommands: () => ({
insertVariableTag:
(variable: string) =>
({ commands }: { commands: Partial<RawCommands> }) => {
return commands.insertContent?.({
type: 'variableTag',
attrs: { variable },
});
},
}),
addCommands: () => ({
insertVariableTag:
(variable: string) =>
({ commands }) => {
commands.insertContent({
type: 'variableTag',
attrs: { variable },
});
return true;
},
}),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤯 I did try the boolean but I still using : { commands: Partial<RawCommands> } as type. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha 😄 I found somewhere in the documentation where they added custom commands and used this syntax.

@Devessier Devessier self-requested a review October 23, 2024 14:45
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.

Perfect!!

@thomtrp thomtrp force-pushed the tt-add-available-variables-dropdown branch 2 times, most recently from 0ea9dcf to ace95a4 Compare October 23, 2024 15:34
@thomtrp thomtrp force-pushed the tt-add-available-variables-dropdown branch from 1de9d77 to f3ec78d Compare October 23, 2024 16:10
@thomtrp thomtrp merged commit 2e8b845 into main Oct 23, 2024
18 checks passed
@thomtrp thomtrp deleted the tt-add-available-variables-dropdown branch October 23, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants