Form builder

[pavish]

This PR contains the following:

• Form builder which takes in a json and renders a form
• DynamicInput component which is an abstraction over all form input components based on data type
  • This would also be the base component for CellInput
• InputGroup component
• TextInput is more normalized and only comprises of input element
• NumberInput component - basic
• Shared styling implementation for component library
• npm audit will run in production mode

Sample json config:

{
  variables: {
    numberType: {
      type: 'string',
      default: 'Integer',
      enum: ['Integer', 'Decimal', 'Float'],
    },
    integerDataSize: {
      type: 'string',
      default: 'default',
      enum: ['default', 'bigInt', 'smallInt'],
    },
    decimalPlaces: {
      type: 'integer',
      default: 2,
    },
    maxDigits: {
      type: 'integer',
      default: 2,
    },
    floatingPointType: {
      type: 'string',
      default: 'real',
      enum: ['real', 'doublePrecision'],
    },
  },
  layout: {
    orientation: 'vertical',
    elements: [
      {
        type: 'input',
        variable: 'numberType',
        label: 'Number Type',
      },
      {
        type: 'switch',
        switch: 'numberType',
        cases: {
          Integer: [{
            type: 'input',
            variable: 'integerDataSize',
            label: 'Integer Data Size',
            inputType: 'select',
            options: {
              default: { label: 'Default (4 bytes)' },
              bigInt: { label: 'Big Integer (8 bytes)' },
              smallInt: { label: 'Small Integer (2 bytes)' },
            },
          }],
          Decimal: [{
            type: 'layout',
            orientation: 'horizontal',
            elements: [
              {
                type: 'input',
                variable: 'decimalPlaces',
                label: 'Decimal Places',
              },
              {
                type: 'input',
                variable: 'maxDigits',
                label: 'Max Digits',
              },
            ],
          }],
          Float: [{
            type: 'input',
            variable: 'floatingPointType',
            label: 'Floating Point Type',
            inputType: 'select',
            options: {
              real: { label: 'Real (6 digits)' },
              doublePrecision: { label: 'Double Precision (15 digits)' },
            },
          }],
        },
      },
    ],
  },
}

The variables property purely represent data related config and would be extended to contain associated validations, formatting etc., in the future.
The layout property instructs our component on how the layout is supposed to appear.

Demos can be viewed by running storybook.

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the master branch of the repository
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no
  visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[seancolsen]

High-level thoughts

• Overall this looks great!
• I like the FormConfiguration structure.
• I think we'll probably want to move all the form-builder components out of the component library before we release the library. I don't think the form-builder is nearly as general-purpose as the component library. Even for someone who wants to build a form from JSON, I suspect that most of the time their needs will differ from ours in subtle ways that make this code tough to re-use. I'd lean towards keeping all this code strictly within the Mathesar codebase. I say leave it where it is for now, but keep this in mind when the time comes to clean up the component library for release.

All independent discussion topics are enumerated with (1), (2), etc.

---

Questions

(1) DynamicInput props type vs inputType

• Why does DynamicInput have separate props for type and inputType? I find this design to be confusing, but I don't fully understand the rationale for keeping these props separate, so I'm open to the possibility that they need to remain separate. Can I make a <DynamicInput type=float inputType=checkbox /> or a <DynamicInput type=time inputType=textarea />? If not, how do I determine the valid combinations of type and inputType?

• My inclination is to merge these two props into one prop that would contain a union type having all the valid combinations of both props.

• If we're going to keep the props separate, I'd like to:

  • Use better names for the props and types.

    Currently we have:

    export let type: DynamicInputType;
    export let inputType: DynamicInputElementType;

    That's especially confusing because if I'm holding those four names in my head, I'm likely to equate the inputType prop with the DynamicInputType type. Matching the types with the props requires more memorization that should be necessary.

    Ideally:

    export let fooType: DynamicInputFooType;
    export let barType: DynamicInputBarType;

    Where "foo" and "bar" are more meaningful words. I'm not sure what those words should be, but I can offer suggestions once I gain a better understanding of the rationale for keeping these props separate.

  • Add some code comments explaining: why the two props are separate; and explaining how to ues them with some examples that illustrate the need for separate props.

---

Fix in this PR

(2) Fix uncommitted changes to package-lock.json

I'm seeing local changes to package-lock.json after restarting the Docker container.

git diff HEAD --shortstat shows 1 file changed, 53 insertions(+), 56 deletions(-)

package-lock.json has md5 d36574608c2dd23cfed5347aedc3baf5

Are you seeing the same thing after restarting your container? If not, we should troubleshoot. If so, I think we should commit those changes.

(3) Fix NumberInput setting value to string

The two-way binding for value should reliably produce a type of number | undefined.

Case 1

1. Instantiate a NumberInput component with a bound value equal to 7.
2. In the browser focus on the input, placing the cursor at the end.
3. Enter 8.
4. Expect value to be 78 (number).
5. Observe value to be "78" (string).

Case 2

1. Instantiate a NumberInput component with a bound value equal to 7.
2. In the browser focus on the input, placing the cursor at the end.
3. Press Backspace once, clearing the contents of the input.
4. Expect value to be undefined.
5. Observe value to be "" (empty string).

Furthermore (and on a related note), I'm a bit confused about what's happening with interface HTMLNumberInputElement, and my hunch is that we should remove it.

My understanding is that the value property of an <input> will always be a string, even when setting type='number'. That means that writing e.target as HTMLNumberInputElement (in NumberInput.svelte) is incorrectly providing type assurance that value will be a number.

I would expect NumberInput to assume that the underlying DOM element will have a string value, and to be converting that string to a number (i.e. with Number(inputElement.value)) to produce a binding to the value prop that satisfies number | undefined.

(4) DRY out code-level docs for DynamicInput

• I've mentioned this before, but I don't like code comments unless they answer a "why" question. I don't like that we have lots of comments on props, just for the sake of Storybook. If it were my codebase I'd remove all of these. However, this is not my codebase, so I've acquiesced to comments on props just for the sake of Storybook.
• The comment on the type prop of DynamicInput goes a step further though, which I think is worth addressing, because it actually duplicates code into the comment. If I'm refactoring the DynamicInputType I'll have no way of knowing I need to update a code comment in a different file. Can we either remove this code comment or change it to something more generic that doesn't have code duplicated in the comment?

(5) In interface FormLayout, make type property required

• Since FormLayout is used in the discriminated union for FormElement, it's discriminating property, type should be required.

(6) Improve naming of some form builder type properties

• In ConditionalSwitchElement, rename switch to variable
• In ConditionalIfElement, rename if to variable

---

Fix in this PR — or track by opening a new ticket

I wrote these comments with an eye towards copy-pasting them into new tickets if needed.

(7) More gracefully handle some edge cases in NumberInput

Independent sub-issues, with steps to reproduce.

• Support pasting multi-character values

  1. Begin with a value of 1
  2. Into your clipboard, copy 22
  3. Focus on the input and select all its contents
  4. Paste
  5. Expect 22
  6. Observe 1

  Adding a + after the group in validKeyRegex appear to fix this.

• Support entering -0.1

  1. Begin with an empty input
  2. Enter -0 and observe -0 which is good
  3. Type .
  4. Expect -0.
  5. Observe 0

• Allow empty integer

  1. Begin with 1 and isInteger
  2. Place cursor at end
  3. Press Backspace once
  4. Expect empty input
  5. Observe 0

• Don't wipe contents when entering 7..

  1. Begin with an empty input
  2. Enter 7. and observe 7. which is good
  3. Type .
  4. Expect 7.
  5. Observe an empty input

• Retain cursor position

  1. Begin with 78
  2. Move cursor between 7 and 8
  3. Enter a
  4. Expect 78 with cursor between 7 and 8
  5. Observe 78 with cursor after 8

• Don't auto-delete decimal

  1. Begin with 1.2
  2. Delete the 2
  3. Expect 1.
  4. Observe 1

• Don't auto-delete minus sign

  1. Begin with -8
  2. Delete the 8
  3. Expect -
  4. Observe an empty input

• Don't truncate integer value when entering decimal midway through number

  1. Begin with 23 and isInteger
  2. Place cursor between 2 and 3
  3. Enter .
  4. Expect 23
  5. Observe 2

• Remove support for numbers like 8e9, or allow users to enter them. I can enter 89, then place my cursor between the two digits and type e. But I can't enter 8e9 directly. I don't think we need support entering these kinds of numbers into the input though.

Additional thoughts

• I'd also like to stress test this a bit more after the string type issue is fixed.
• It might be worth using the beforeinput event instead of the input event.

(8) Add tests for NumberInput

This is the kind of component that really benefits from having some tests in my opinion.

We could pretty easily write tests for the scenarios I've laid out above, as well as other ones that already work correctly.

(9) Remove type='number' on NumberInput

I hate the browser native number input, and I think most people do too.

• I don't like the tiny up/down buttons. They look bad and don't add much value.
• The value of the input changes in response to scroll events, which can be really annoying sometimes.

I'd prefer that we remove

Considerations:

• We'll need to set inputmode to either decimal or numeric, potentially with some logic based on the props.
• We'll need to adjust some of the logic within the onInput function to account for the different behavior of inputElement.value when the user enters a non-numeric string like x into the input.

(10) Remove $$restProps on BaseInput component instances

There are 6 occurrences of <BaseInput {...$$restProps}. I'd rather changes this to <BaseInput {labelController} (and add a labelController prop to the component which consumes BaseInput).

Rationale:

• Props serve as documentation to help developers use the components. The fact that TextInput now accepts a labelController prop is quite opaque. Further, we lose type safety when passing labelController values into TextInput because the type checker also doesn't understand that TextInput accepts a LabelController in its labelController prop.
• I've used {...$$restProps} in exactly this way before (to reduce code duplication) and I've ended up regretting it later. I'd rather sacrifice some DRY principles for the sake of being more explicit with our props.
• I'm okay with {...$$restProps} as its used in Button.svelte because it's placed on a DOM element. In that case, it's clear to the developer that the Button component can accept arbitrary DOM attributes like disabled. I'm also okay with it in DynamicInput.svelte because it's harder to avoid in that situation. But I'd like to avoid using {...$$restProps} on custom components if we can easily do so.

(11) Improve focus indicator of InputGroup component

I really like the changes that simplify TextInput to a single DOM element, and I want to make sure to keep those changes. There's also clearly value in keeping the "grouped input" functionality. But the changes in this PR lead to a minor stylistic regressions in my opinion.

Examples:

• Filter tables

  Before	After

• Add column

  Before	After

I'd prefer that the focus indicator encompass the entire group, like it did before.

I'd like to keep the changes that add a grey background to the group items as it helps to distinguish the label from the input.

In Storybook, I see that InputGroup is also intended to handle the use case of multiple inputs grouped together, in which case the focus indicators would need to be separate. But this use-case seems quite esoteric to me. I can't recall ever having seen a UI like that. I'd suggest we abandon any attempts to support it.

(12) Remove !important in component library CSS

The InputGroup component introduces CSS which uses !important. I'm fine using !important in Mathesar CSS, but I think we should strictly avoid it within the component library since we are aiming for components which can be re-styled.

I haven't yet thought through a strategy for eliminating !important in this case, but I can help figure it out if needed.

[pavish]

(1) DynamicInput props type vs inputType

Why does DynamicInput have separate props for type and inputType?

type refers to data type of the value
inputType refers to the input element type

Normal HTML inputs and our input components which wrap them are not optimized for the data type of their value, rather they focus on how the user would work with the inputs. This is great when constructing UI but not when trying to generalize it or perform any data type specific operations. The DynamicInput component's major purpose is not just to have a unified component wrapping all inputs but to have an input component that emphasizes on the values and their data types.

• This helps with creating a declarative format of data obtained from forms where the input element type is a presentational concern and not a functional concern.
• The primary use of this component is with the form-builder and CellInput, both of which would greatly benefit by using data type instead of element type.
• We can extend this component to perform validations and formatting of value, and have strict constraints around them since we know the data type.

With the above idea in mind, it made more sense to have a mandatory type prop that only allows options that we can consider as primitive data types: 'boolean' | 'integer' | 'float' | 'string' | 'date' | 'datetime' | 'time'. This is the only functional prop we need for the component to work.

Now arises a different need that is purely presentational, being able to select the element type i.e inputType. For eg., we might want to prefer showing a dropdown or a toggle for selecting a boolean value. This has no functional need and is an optional configuration.

If not, how do I determine the valid combinations of type and inputType?

Only by documentation, comments or by looking through the code.

Unfortunately, we cannot enforce combinations here with Typescript. Right now if the inputType is invalid for the data type, the component silently ignores it and renders it only by data type.

We could perhaps explicitly add a validation method which prints warnings if you think it would make it easier for developers.

My inclination is to merge these two props into one prop that would contain a union type having all the valid combinations of both props.

Having a single prop that's a union of both would go against the reasons for this component's existence and put a shortstop to the benefits it offers and possible future extensions.

Use better names for the props and types.

How about we rename type to dataType. I'm however, inclined to just keep it as type.
We could rename inputType to elementType.

I'm open to suggestions.

Add some code comments explaining: why the two props are separate

I think it would be best to just add a code comment with a link to this discussion, if you also feel that would be enough.

[pavish]

(2) Fix uncommitted changes to package-lock.json

Fixed in 0a30de5

(4) DRY out code-level docs for DynamicInput

I don't like that we have lots of comments on props, just for the sake of Storybook.

I understand and share the frustation of having to do this for the sake of storybook but until we release the component library and have a separate site built for it with sveltekit, I'd rather have us keep it. As of now (and we've seen this happen), Storybook serves as a starting point for potential contributors and I think such comments add enough value in community building aspects, where they lack value in informational aspects.

Can we either remove this code comment or change it to something more generic that doesn't have code duplicated in the comment?

This comment exists because it helps with intellisense and Storybook documentation. We could remove it here, but we would still need to document it. So, I don't see a harm in keeping it here for now. I'm open to suggestions on this.

(5) In interface FormLayout, make type property required

Since FormLayout is used in the discriminated union for FormElement, it's discriminating property, type should be required.

Is this more of a personal preference or is there a stronger reason for this?

Discriminated union works fine for FormElement because type property differs across the types, and one of the differing aspect is that it is optional for FormLayout.

The way I see it is that FormLayout is usually the first layout element and I wanted to avoid always having to include type for it.

I am totally okay with making it required by the way. I will wait for your reply before making the changes.

(6) Improve naming of some form builder type properties

Fixed in bc19fa5

[pavish]

(3) (7) (8) (9) Everything related to NumberInput

I'm aware of NumberInput being pretty much useless at this point. It is more of a placeholder component and I'd like us to work on it separately with a dedicated issue. I've created #970 for this.

I would also like to fix the problems mentioned under Fix in this PR as part the new issue since this PR is already large and serves as a base to a number of future PRs. On the other hand, NumberInput can be worked upon in parallel without blocking anything else.

[pavish]

(10) Remove $$restProps on BaseInput component instances

The fact that TextInput now accepts a labelController prop is quite opaque.

Not for long. Svelte language tools currently support an experimental feature to explicitly specify types for props and this is enabled by default. In essence we could just add:

<script>
  interface $$Props extends BaseInputProps, HTMLInputElement {
    value: unknown
  }
</script>

This makes sure that {...$$restProps} would include all props mentioned in BaseInputProps and HTMLInputElement excluding the ones explicitly mentioned.

Refer:

• TypeScript: Explicitly Typing Props/Slots/Events + Generics sveltejs/rfcs#38 (comment)
• (feat) auto-add types to exports when $$Props used sveltejs/language-tools#1212

I've used {...$$restProps} in exactly this way before (to reduce code duplication) and I've ended up regretting it later. I'd rather sacrifice some DRY principles for the sake of being more explicit with our props.

I'd suggest to keep using {...$$restProps} since I expect the above mentioned experimental features to become mainstream with sveltekit's initial stable release.

[pavish]

(11) (12) InputGroup component

Improve focus indicator of InputGroup component

I am aware of the stylistic regression for the focus indicator. The problem is inorder for the focus indicator to be shown on the InputGroup it needs to be bound from within the slot to the parent. While it is entirely possible to bind it down as a slot prop, it seemed a bit hacky but I still have it under consideration.

In Storybook, I see that InputGroup is also intended to handle the use case of multiple inputs grouped together

I took Bootstrap's InputGroup as an inspiration and constructed our component in a similar way. We can further analyze this. I have come across cases where Select and TextInput are grouped together.

Remove !important in component library CSS

Agreed, but the case in InputGroup required it. We can analyze ways to eliminate it.

I've created #971 for this.

[seancolsen]

@pavish

Discussion topic status

checked = resolved

• (1) DynamicInput props type vs inputType
• (2) Fix uncommitted changes to package-lock.json
• (3) Fix NumberInput setting value to string
• (4) DRY out code-level docs for DynamicInput
• (5) In interface FormLayout, make type property required
• (6) Improve naming of some form builder type properties
• (7) More gracefully handle some edge cases in NumberInput
• (8) Add tests for NumberInput
• (9) Remove type='number' on NumberInput
• (10) Remove $$restProps on BaseInput component instances
• (11) Improve focus indicator of InputGroup component
• (12) Remove !important in component library CSS

---

(1) DynamicInput props type vs inputType

Okay, for now I'll support keeping the props separate. I think I'll better understand your vision for the usage of DynamicInput once I see it in action in subsequent PRs. If I still have concerns about the separate props at that point I'll resume this discussion.

For now though, I would like to use clearer names for the props and their respective types. I suggest consistently using the words "data" and "interface" as follows:

export let dataType: DynamicInputDataType;
export let interfaceType: DynamicInputInterfaceType = undefined;

Specifically:

• Rename DynamicInputType -> DynamicInputDataType
• Rename type -> dataType
• Rename inputType -> interfaceType
• Rename DynamicInputElementType -> DynamicInputInterfaceType

I'm okay with "element" instead of "interface", but I have a slight preference for "interface" because "toggle" isn't actually a DOM element. I also considered the words "widget" and "ui" and would be okay with those too.

(2) Fix uncommitted changes to package-lock.json

I'm still seeing changes to package-lock.json when restarting the docker container.

There are fewer changes than before though. This is the diff against master after I restart docker.

diff --git a/mathesar_ui/package-lock.json b/mathesar_ui/package-lock.json
index 5995cf63..afb770bd 100644
--- a/mathesar_ui/package-lock.json
+++ b/mathesar_ui/package-lock.json
@@ -14589,12 +14589,12 @@
               },
               "dependencies": {
                 "is-glob": {
-                  "version": "3.1.0",
-                  "resolved": "https://registry.npmjs.org/is-glob/-/is-glob-3.1.0.tgz",
-                  "integrity": "sha1-e6WuJCF4BKxwcHuWkiVnSGzD6Eo=",
+                  "version": "4.0.3",
+                  "resolved": "https://registry.npmjs.org/is-glob/-/is-glob-4.0.3.tgz",
+                  "integrity": "sha512-xelSayHH36ZgE7ZWhli7pW34hNbNl8Ojv5KVmkJD4hBdD3th8Tfk9vYasLM+mXWOZhFkgZfxhLSnrwRr4elSSg==",
                   "dev": true,
                   "requires": {
-                    "is-extglob": "^2.1.0"
+                    "is-extglob": "^2.1.1"
                   }
                 }
               }
@@ -17170,6 +17170,23 @@
           "dev": true,
           "requires": {
             "is-glob": "^4.0.1"
+          },
+          "dependencies": {
+            "is-extglob": {
+              "version": "2.1.1",
+              "resolved": "https://registry.npmjs.org/is-extglob/-/is-extglob-2.1.1.tgz",
+              "integrity": "sha1-qIwCU1eR8C7TfHahueqXc8gz+MI=",
+              "dev": true
+            },
+            "is-glob": {
+              "version": "4.0.3",
+              "resolved": "https://registry.npmjs.org/is-glob/-/is-glob-4.0.3.tgz",
+              "integrity": "sha512-xelSayHH36ZgE7ZWhli7pW34hNbNl8Ojv5KVmkJD4hBdD3th8Tfk9vYasLM+mXWOZhFkgZfxhLSnrwRr4elSSg==",
+              "dev": true,
+              "requires": {
+                "is-extglob": "^2.1.1"
+              }
+            }
           }
         },
         "is-extglob": {
@@ -24923,13 +24940,13 @@
               },
               "dependencies": {
                 "is-glob": {
-                  "version": "3.1.0",
-                  "resolved": "https://registry.npmjs.org/is-glob/-/is-glob-3.1.0.tgz",
-                  "integrity": "sha1-e6WuJCF4BKxwcHuWkiVnSGzD6Eo=",
+                  "version": "4.0.3",
+                  "resolved": "https://registry.npmjs.org/is-glob/-/is-glob-4.0.3.tgz",
+                  "integrity": "sha512-xelSayHH36ZgE7ZWhli7pW34hNbNl8Ojv5KVmkJD4hBdD3th8Tfk9vYasLM+mXWOZhFkgZfxhLSnrwRr4elSSg==",
                   "dev": true,
                   "optional": true,
                   "requires": {
-                    "is-extglob": "^2.1.0"
+                    "is-extglob": "^2.1.1"
                   }
                 }
               }

(4) DRY out code-level docs for DynamicInput

On principle, I do feel somewhat strongly about this one. While I certainly don't want to hold up an important PR for the sake of a trivial comment, I'm continuing this discussion for the sake of establishing healthy patterns in our code.

I don't see a harm in keeping it here for now

Here's the harm I see (most important listed first):

• Code comments like this lead new devs astray by encouraging them to look in the wrong places for information. I've been there before and it's been frustrating. If a junior dev doesn't understand how to look up our TS type definitions, then this one code comment will help them -- but only once. That help will prove to be quite superficial once they need to find the type for another prop that's not documented in the same way. At that point they'll need to go looking for the TS type definitions, but that need will not be as clear as it would have been had they needed to look at the TS type definitions in the first place. The lack of clarity surrounding the need to look at TS type definitions is why I feel code comments like this actually have a net-negative effect on welcoming new contributors to our codebase.
• I'd say the risk is quite high of the content in the code comment falling out of sync with the actual type definition, especially given that they are in different files. It's fine now, but there's a risk of harm if it going out of sync because incorrect documentation is worse than missing documentation in my opinion.
• Code comments like this encourage other people to follow this pattern which is more work to write.

(5) In interface FormLayout, make type property required

Is this more of a personal preference or is there a stronger reason for this?

My rationale is somewhere in between a personal preference and a strong reason. I see this as a minor code-cleanliness/readability/maintainability issue.

The discriminated union pattern is so common in TS, and this code almost matches the archetypal pattern but not quite. The code that's there should work fine. But another dev might quickly read that code and fail to notice the optional property. Then they might naively write:

function doSomething(formElement: FormElement) {
  if (formElement.type === 'layout') {
    // I assume formElement is a FormLayout
  } else {
    // I assume formElement is not a FormLayout
  }
}

The above code looks fine but has a bug because if formElement is a FormLayout without a type property then those assumptions will be incorrect.

I wanted to avoid always having to include type for it

With your approach, it's a tiny bit less code. I can see how that's nice. With my approach it's a tiny bit more code. But I still prefer my approach because it's more explicit and I think carries a tiny bit less risk of future bugs.

All that said, I'm fine to let this one go if you want to keep it as-is. I don't feel particularly strong about it.

(10) Remove $$restProps on BaseInput component instances

That's cool that Svelte language tools will eventually be able to provide type safety here.

I still have a slight preference for being more explicit in cases like this. The additional prop adds very little additional code and it adds value by better documenting the behavior of the input components to developers who use them. For the sake of time though, I'm fine moving forward without any additional changes.

Reflections on patterns for documenting component props

I'm a little confused when juxtaposing your opinions on the following two discussion topics:

• (4) DRY out code-level docs for DynamicInput
• (10) Remove $$restProps on BaseInput component instances

Reflections:

• At the heart of both topics is a question about how best to document a component's props.
• In both cases I want to see code for the prop and no code comments. I like using code to document props because we can use tooling to refactor the code and automated tests to validate the code.
• In (4) you want to provide a lot of props documentation -- going so far as to duplicate code from another file into a code comment. Whereas in (10) you want to provide no props documentation -- leaving developers without a clear indication that components like Checkbox and TextInput accept a labelController prop. I fined these two opinions inconsistent. Can you help me understand your logic?

[pavish]

(1) DynamicInput props type vs inputType

I've renamed the props in 51ca4dd

(2) Fix uncommitted changes to package-lock.json

I missed this earlier. Fixed in 6d5c9e5

[pavish]

(5) In interface FormLayout, make type property required

Here's the reason on why I made it optional. The FormConfiguration interface is as follows:

export interface FormConfiguration {
  variables: Record<string, {
    type: DynamicInputDataType,
    default: FormInputDataType,
    enum?: unknown[]
  }>,
  layout: FormLayout
}

The layout property always expects a FormLayout. It is essential that the layout property is of type FormLayout and not any FormElement inorder to construct the ui. Now, an example configuration for this would look as follows, if type is made mandatory:

  variables: { ... },
  layout: {
    type: 'layout',
    orientation: 'vertical',

Having type: layout within layout gives an impression that this infact allows any FormElement. Even though, we'd be able to impose this strictly on the frontend, this configuration is supposed to be provided from the backend in the future, where an unsuspecting plugin developer might directly provide a type: 'input' here which would not work on the frontend.

Having the type as mandatory would help frontend devs a little, but I think it might confuse users and backend devs. If we are to make it mandatory, we may have to come up with a whole new format.

[pavish]

(4) DRY out code-level docs for DynamicInput

Let me clarify that I share your frustration on this and strictly speaking from our code perspective, these comments are quite useless and I agree with all your points.

But I have a few reasons why I think this should be present for now (Notice the for now, we would eventually remove all these comments).

Purpose of our Storybook setup

• Our current Storybook setup is not mainly for developer reference on the Mathesar repo, but rather to serve as basic documentation for early adopters of our component library.
• When we release the alpha of the library, we would probably not have time to build an entire site using sveltekit dedicated for documentation, and the easiest and fastest would be host our storybook for early users to reference. This was actually the main reason why we included storybook in the first place. Until the site is ready, this would be the only source of documentation.
• These comments on props only exist for the sake of Storybook.

Why we have comments and the weird .mdx files

1. Considering that we only have Storybook for now without these comments. Without referring our codebase, it's quite hard to understand what the props represent.

   • The props for dropdown component do not have comments and this is the argument table in the storybook doc for it.
   • 
   • While it is quite clear for many of the props, it is hard to understand what some of these represent eg., closeOnInnerClick, and the possible allowed values for props such as placement.
   • We don't expect users of the library to have to go through our codebase to just use the components.
   • Adding comments provides some information on these props to the user.

2. You might have seen some stories using .mdx files. I added them for the same purpose as being able to provide atleast the basic usage information, and add further documenation later on without messing with stories.

   • Just looking at the Confirmation system on storybook does not provide any idea on how to actually use it.
   • 
   • It doesn't answer questions like, where does confirmationController come from? What are the operations possible on it? And what do the methods like handleRecycle do?
   • In comparison, the Form system provides the following:
   • 
   • 
   • While this may not be well detailed, it does answer questions on how to use the component for someone looking at this without having to go through our code.

Regarding,

• I'd say the risk is quite high of the content in the code comment falling out of sync with the actual type definition,

  • Yes, there is quite a risk of it falling out of sync but when we have a documenation site setup where we list all our props, the risk would still be present except it will be between our code base and the documenation. This is something we cannot avoid. Right now, I only think of these code comment as end-user documentation.

When we finally work on the documentation site, we will just remove them from code and copy it all over to the site and improve upon that.

I'm not sure if this convinces you but unless we find a better make-shift documentation option for when we release the alpha version for our component library, I don't see an option other than having to work with Storybook and to add these comments.

[pavish]

Reflections on patterns for documenting component props

I've given my reasons on the commenting props for (4) in #915 (comment).

I am all in for providing documentation but (10) on the other hand is a whole other problem.

1. Regarding documenting labelController prop in each input component: (For users)

   • LabelController is not exported out of our library. It currently exists only as an internal class.
   • Our current design does not allow labelController to be passed down directly to the input components without having to use the Label component.
   • Any documenation on LabelController would be covered by the Label component and possibly a separate section or it, but it would not fall into each individual input component.
   • We would also be documenting BaseInput which will cover all the common props for input components.

2. Regarding documenting it for devs working on the mathesar repo:

   • I expect Svelte to support it out-of-the-box as mentioned in the comment: Form builder #915 (comment)
   • Manually specifying this prop right now may seem like a small amount of work. But this has to be specified for each input component: TextArea, TextInput, NumberInput, Checkbox, Radio, Select etc.,
   • I expect BaseInput component to have more props and methods in the future.
   • If we move this to all the input components for now, refactoring it at a later point in time to use restProps would end up being a lot of work, which we can avoid by putting up with a minor inconvenience for a considerably short period of time.

[seancolsen]

(1) DynamicInput props type vs inputType

• Your changes look good. Thanks.

(2) Fix uncommitted changes to package-lock.json

• This works for me now. All set here.

(4) DRY out code-level docs for DynamicInput

• I'm actually even a tad more concerned about this approach after reading your latest blurb. 😕 However, given that we still disagree after some significant debate on a relatively insignificant topic, I will respectfully defer to you as the more senior engineer so that we can keep moving. No further changes needed.

(5) In interface FormLayout, make type property required

• Ok, I'm convinced by the argument with respect to the back-end. No further changes needed

(10) Remove $$restProps on BaseInput component instances

LabelController is not exported out of our library. It currently exists only as an internal class.

That's my bad. I would like it to be exported. I'm still adjusting to the pattern of using index.ts and I often forget to put stuff in there.

Our current design does not allow labelController to be passed down directly to the input components without having to use the Label component.

My intention with the design of the Label system is for input components to have the option of receiving a controller directly. I even created a story for it here: http://localhost:6006/iframe.html?id=components-label--manually-created-controller&args=&viewMode=story

In Label.stories.svelte we have:

<ul>
  <li><Label {controller}>First label</Label></li>
  <li><Checkbox labelController={controller} /></li>
  <li><Label {controller}>Second label</Label></li>
</ul>

This story exemplifies a scenario with two properties that both require manually passing the controller to the input:

• The input cannot be nested inside the label due to the desired DOM structure
• Multiple labels are associate with one input

While the need for those situations will be uncommon, I think we should handle them since we want the component library to be general purpose. We could manually pass an id instead of the controller, but then we would lose the extra features which the controller brings, like formatting the label to indicate when the input is disabled.

We would also be documenting BaseInput which will cover all the common props for input components.

The problem with relying on the documentation within BaseInput is that it lacks discoverability. When inspecting the Checkbox component, devs won't know to look at the props for BaseInput. A clever dev might notice that Checkbox.svelte contains {...$$restProps} and then go looking at BaseInput, but it's quite hidden. Most people won't notice.

If you're swayed by the above arguments we can handle the changes in a new ticket. I'd like to add LabelController to index.ts anyway, and that could be part of the same PR. I'd be happy to make the changes.

[seancolsen]

I've set this to merge on pass.

Excited to be getting this in! Fantastic work on this one, @pavish 🎉 I can tell you put a lot of thought into designing the JSON stuff and I think it's going to be awesome!

[pavish]

@seancolsen Thanks for setting this to merge on pass but you forgot to approve the PR. :)

I'd rather have us reach a consensus that both of us are atleast partially satisfied on, for:

• (4) DRY out code-level docs for DynamicInput
  • We could discuss abandoning Storybook. But we might need to find an alternate documentation strategy that we can use right away.
• (10) Remove $$restProps on BaseInput component instances

  • When inspecting the Checkbox component, devs won't know to look at the props for BaseInput

  • They would, when we introduce the interface $$Props option that we expect Svelte to support
  • But I think this warrants more discussion to establish a pattern around it, since this would probably come up in several places again.

It would be better to create separate issues for both of these and have them as agenda items on our next UI sync call.

[seancolsen]

Thanks for setting this to merge on pass but you forgot to approve the PR. :)

🤣 Oh dear

Yeah let's chat about those remaining two topic on our next sync call.