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

Migrate trigger form params to new UI #45270

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

Conversation

jscheffl
Copy link
Contributor

@jscheffl jscheffl commented Dec 29, 2024

After @shubhamraj-git has added the trigger form to the new React UI, this PR now adds the flexible trigger form - Based on the logic of the former AIP-50 implementation - using DAG ParamsDict to render form fields based on field definitions.

Note that the form rendering was made as a component by intent as it will be re-used in Connection Forms in future to render connection extra fields.

To test the form rendering the following DAGs might be good candidates:

  • example_bash_decorator - has no params so should not display a specific form
    image

  • example_bash_operator - has one basic parameter w/o any special thing. Simple as it can be
    image

  • example_params_trigger_ui - has 4 parameters, still basic
    image
    ... and with advanced section un-folded:
    image

  • example_params_ui_tutorial - this is the most complex beast as it has all features as a demo that shall be supported.
    image
    image
    ...and the lower section un-folded:
    image

Scope of the PR is the UI layout and logic to render. What is missing and will be a follow-up PR is the binding and sync of the form field content with the JSON dict that is used to trigger the DAG in the "Advanced Options" section.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Dec 29, 2024
@jscheffl jscheffl added the AIP-50 Trigger DAG UI user Form label Dec 29, 2024
@jscheffl jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch from 51d3c4d to 83c450e Compare December 29, 2024 22:21
@shubhamraj-git
Copy link
Contributor

shubhamraj-git commented Dec 30, 2024

Thanks @jscheffl for working on this.
Had the first look. Looks great. The base is almost ready.

As mentioned in TODO, we will support the accordion and not flat.

Some points which i noticed, just writing it down incase we don't miss.

  1. There is no live changes to conf json onBlur
  2. Should have a validation for integer inputs.
  3. Dropdown not working for select values
  4. we should have a time picker.
  5. Length check not working
    6. JSON inputs should have a code view? (Just a opinion)
  6. List should have multi lines

Overall this is going in right direction.

Should we have inbuilt scroll for the dynamic form? since the modal seems very long.

EDIT: Just saw a todo for point 6 (json code view) and most of the above issues as todo.

@jscheffl jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch 4 times, most recently from e7381e7 to 8067918 Compare January 2, 2025 15:41
@jscheffl
Copy link
Contributor Author

jscheffl commented Jan 2, 2025

Thanks @jscheffl for working on this. Had the first look. Looks great. The base is almost ready.

As mentioned in TODO, we will support the accordion and not flat.

Some points which i noticed, just writing it down incase we don't miss.

1. There is no live changes to conf json onBlur
2. Should have a validation for integer inputs.
3. Dropdown not working for select values
4. we should have a time picker.
5. Length check not working
   ~6. JSON inputs should have a code view? (Just a opinion)~
6. List should have multi lines

Overall this is going in right direction.

Should we have inbuilt scroll for the dynamic form? since the modal seems very long.

EDIT: Just saw a todo for point 6 (json code view) and most of the above issues as todo.

(1) I recommend to make into a separate PR - as also discussed 1:1
(2,3,5,6) fixed.
(4) would need a component fix in react for Chrome... and a workaround for Firefox. Firfox time picking is "just broken" - would make this to a separate PR as well.

So... for the Layout part I'd say... ready for review!

@jscheffl jscheffl marked this pull request as ready for review January 2, 2025 15:52
Copy link
Contributor

@shubhamraj-git shubhamraj-git left a comment

Choose a reason for hiding this comment

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

Thanks @jscheffl ! Overall it looks good. Just few comments.
When clicking on trigger run, the modal pops up with all the options at front.
Should we consider them in a accordion and if needed user can view?
Apart from it, had some general comments, which is similar for all files.


export const FlexibleFormFieldDate = ({ name, param }: FlexibleFormElementProps) => (
<Input
defaultValue={param.value as string}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check the type of param.value or it will be for sure string?
If No, we should consider using typeof param.value === "string" ? param.value : "";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a bit of safety net in all default parameters taking this from param.value - can you take a look again?

@jscheffl jscheffl force-pushed the feature/migrate-trigger-form-params-to-new-ui branch from 8067918 to 313d868 Compare January 4, 2025 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-50 Trigger DAG UI user Form area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants