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(form): Enable 'Required' field feature in the canvas form #1283

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

shivamG640
Copy link
Contributor

@shivamG640 shivamG640 commented Aug 1, 2024

Fixes #1265

Notes of what has changed:

  1. Added Required tab for all kind of fields (including dataformat, Loadbalancer etc).
  2. Renamed the Tabs display name. {All Fields -> All, User Modified -> Modified}
  3. Changed the order of the Tabs {Required -> All -> Modified}
  4. Move the Tabs section to the top of the side bar.
  5. Add proper Tests for the functionality.
  6. Add Tooltips to tabs
  7. Fix failing E2e tests.
  8. Add code stating that there are no fields matching this criteria when the tab is empty.
  9. Add changes to persist tab selection across steps.
  10. Reorder components in the side bar.
  11. Make the tabs equal size + button text bold.

Notes of what needs to be added further:

  • Change to css of tab to make it look like component type in the catalog modal.

@shivamG640
Copy link
Contributor Author

Hi @lordrip
Please have a look at the functionality specially for dataformat and similar fields here.

@lhein
Copy link
Contributor

lhein commented Aug 2, 2024

I am wondering if for data formats and alike we should also show the other fields which belong to the choice you make. Maybe worth a discussion.

@shivamG640
Copy link
Contributor Author

I am wondering if for data formats and alike we should also show the other fields which belong to the choice you make. Maybe worth a discussion.

I have a mixed feeling on that and therefore I stressed on checking the behavior of those special editors fields. @lordrip What do you think?

@shivamG640
Copy link
Contributor Author

We also need to keep in mind that some components doesn't have any required field so showing the 'Required' tab by default when user selects a node can show and empty form like this. @lhein
image

@lhein
Copy link
Contributor

lhein commented Aug 3, 2024

In case of no required or modified fields (empty tabs) we should display a label instead stating that there are no fields matching this criteria.

@lordrip
Copy link
Member

lordrip commented Aug 5, 2024

@shivamG640 @lhein

In regards to showing dependant required fields:
My proposal would be for primitive fields, we show them if required as this is the most straightforward case, but for objects, as soon as we identify the parent as required we bring the entire object.

For LoadBalancerEditor, ExpressionEditor, or DataformatEditor editors, this will mean that they'll appear in the Required section always.

But at the same time, we need to consider what this would mean for other properties since this would mean that a complex object will show all its properties.

This wouldn't be my favorite solution as I feel it a bit strange, but nevertheless, I'll share it, we might add an additional step to my previous suggestion that checks whether the object property is empty or not, if it's empty, it means we can show the entire object as we don't know what would be important for the user, in the contrary, we show only the required children.

Let me elaborate with a few examples:

Example 1

We show name and age, as both are primitive and required.

{
  "properties": {
    "name": {
      "type": "string"
    },
    "age": {
      "type": "integer"
    },
    "isStudent": {
      "type": "boolean"
    }
  },
  "required": ["name", "age"]
}

Example 2

We show expression > value since expression is required as well as the value property

{
  "properties": {
    "expression": {
      "type": "object",
      "properties": {
        "operator": {
          "type": "string"
        },
        "value": {
          "type": "string"
        }
      },
      "required": ["value"]
    }
  },
  "required": ["expression"]
}

Example 3

We show all properties, as expression doesn't define any required property

{
  "properties": {
    "expression": {
      "type": "object",
      "properties": {
        "operator": {
          "type": "string"
        },
        "value": {
          "type": "string"
        }
      },
      "required": []
    }
  },
  "required": ["expression"]
}

@shivamG640
Copy link
Contributor Author

@shivamG640 @lhein

In regards to showing dependant required fields: My proposal would be for primitive fields, we show them if required as this is the most straightforward case, but for objects, as soon as we identify the parent as required we bring the entire object.

For LoadBalancerEditor, ExpressionEditor, or DataformatEditor editors, this will mean that they'll appear in the Required section always.

But at the same time, we need to consider what this would mean for other properties since this would mean that a complex object will show all its properties.

This wouldn't be my favorite solution as I feel it a bit strange, but nevertheless, I'll share it, we might add an additional step to my previous suggestion that checks whether the object property is empty or not, if it's empty, it means we can show the entire object as we don't know what would be important for the user, in the contrary, we show only the required children.

Let me elaborate with a few examples:

Example 1

We show name and age, as both are primitive and required.

{
  "properties": {
    "name": {
      "type": "string"
    },
    "age": {
      "type": "integer"
    },
    "isStudent": {
      "type": "boolean"
    }
  },
  "required": ["name", "age"]
}

Example 2

We show expression > value since expression is required as well as the value property

{
  "properties": {
    "expression": {
      "type": "object",
      "properties": {
        "operator": {
          "type": "string"
        },
        "value": {
          "type": "string"
        }
      },
      "required": ["value"]
    }
  },
  "required": ["expression"]
}

Example 3

We show all properties, as expression doesn't define any required property

{
  "properties": {
    "expression": {
      "type": "object",
      "properties": {
        "operator": {
          "type": "string"
        },
        "value": {
          "type": "string"
        }
      },
      "required": []
    }
  },
  "required": ["expression"]
}

This approach looks quite confusing to me as a user, I might feel like all the fields are required but that is not the real case. Moreover, so far, we've followed the approach to load the object field completely is the object is marked as required so following the same approach would be kind of consistent IMO.

@shivamG640 shivamG640 force-pushed the Feat_1265 branch 3 times, most recently from dada36d to a13bdfe Compare August 8, 2024 20:32
Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

LGTM, I like the form header being sticky and how we can navigate from the empty tab to All 👌

Maybe we should rephrase the "No field found matching this criteria. Please switch to the tab." wording, but until we have another proposal, let's keep it as it is.

@shivamG640 shivamG640 force-pushed the Feat_1265 branch 5 times, most recently from f5701ee to 1323efb Compare August 12, 2024 12:21
@shivamG640 shivamG640 marked this pull request as ready for review August 12, 2024 22:06
@lordrip lordrip merged commit 7b447cc into KaotoIO:main Aug 12, 2024
10 checks passed
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.

Introduce a new config form Tab called "Required Fields"
3 participants