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 support for enum Values in Attributes/Properties #51

Open
jogibear9988 opened this issue May 8, 2021 · 13 comments
Open

Add support for enum Values in Attributes/Properties #51

jogibear9988 opened this issue May 8, 2021 · 13 comments

Comments

@jogibear9988
Copy link
Contributor

So the manifest should list the possible values, so a designer could use them.
also a min/max for a number could be usefull

I've the following Properties for a Property in my designer:
https://github.com/node-projects/web-component-designer/blob/master/src/elements/services/propertiesService/IProperty.ts

@jogibear9988
Copy link
Contributor Author

#52

@justinfagnani
Copy link
Collaborator

I think that if it's valuable to have enums for attributes, it'd also be valuable for properties (and methods parameters, etc), especially considering that most custom element attributes are going to be associated with properties. So I think we should probably encode this into the Type interface... however there's a very slippery slope when dealing with types on how much we encode into the manifest directly and how much we leave to a full type system like TypeScript's.

So far we've really tried to limit how much of a type system we're building in the manifest. We obviously describe classes, and functions, but the types of parameters and fields, etc. are left up to simple string descriptions with references out to other files like .d.ts files. I think it'd be good to preserve this approach in general, but one area where it falls short is in describing interfaces, enums, and constraints that are not even encodable yet in TypeScript, but might be useful for tools.

I do think we should consider adding interfaces as a supported kind, and enums make sense to me too given their prevalence in attributes. Constraints maybe could fit within extensions... I'm not sure we'd be able to cover all of the constraints that a tool might need.

So maybe we should add a "type" declaration kind. A type could be an interface, enum, or just a type string that's referenced by name from other type fields.

A common type string definition:

{
    "kind": "type",
    "name": "OptionalFooArray",
    "text": "Array<Foo> | undefined"
}

An enum

{
    "kind": "type",
    "name": "OneOrTwoEnum",
    "values": [
      {
        "value": "one",
        "label": "One",
      },
      {
        "value": "two",
        "label": "Two",
      }
    ]
}

An interface:

{
    "kind": "type",
    "name": "",
    "members": [
      {
        "kind": "field",
        "name": "foo",
        "type": "string"
      },
      {
        "kind": "method",
        "name": "bar",
        "parameters": [
          {
            "name": "baz",
            "type": "number",
          }
        ],
        "return": {
          "type": "string"
        }
      },
    ]
}

This may look like an increase in complexity for the manifest format, but I think that an interface kind is just a subset of the class kind and if we didn't add an interface kind, people would just uses classes for that purpose. A common text type kind seems good just from a deduplication standpoint. Otherwise manifest generators will have to expand and inline complex named types, or assume that consuming tools can resolve source references.

For enums I can see how they're useful for tools like designers and storybook-like tools, but they do open a door towards more and more complex type object in this format... another option is to have the tool assume TypeScript type syntax and parse and read the type strings in the manifest. An enum would then just be 'one' | 'two' | 'three', etc.

cc @thepassle @rictic

@jogibear9988
Copy link
Contributor Author

If we would enums only in this format:

   'one' | 'two' | 'three'

It would not capture number enums, where each number would have a different meaning.

@jogibear9988
Copy link
Contributor Author

jogibear9988 commented May 27, 2021

@justinfagnani

if we do this

    {
        "kind": "type",
        "name": "OneOrTwoEnum",
        "values": [
          {
            "value": "one",
            "label": "One",
          },
          {
            "value": "two",
            "label": "Two",
          }
        ]
    }

shouldn't we add a description for the enum values?

@thepassle
Copy link
Collaborator

How would a developer document this, so that tools can pick up on the enum?

@jogibear9988
Copy link
Contributor Author

i would think like this:

/**
 * BlaBla
 */
export enum aa{
  /**
   * Description 1
   */
  'abc' = 1,
  'def' = 2
}

@thepassle
Copy link
Collaborator

How would that get correctly linked to an attribute?

@jogibear9988
Copy link
Contributor Author

I don't get your question?

How is any property correctly linked to a attribute?

in lit you could use
@Property({type: aa})

@jogibear9988
Copy link
Contributor Author

What about a possible "Values" structure like this:

  export interface ValueDescription
  {
       name: string,
       min ?: number|string, //optional, needed?
       max ?: number|string, //optional, needed?
       values: [
              {
                    value: number|string|bigint|all js types,
                    name?: string, // a short name of the value, for example if the value is of type number, what means 1 what 2,...
                    description?: string
              }, ...
          ]
  }

and then every definition could link to such a description via the name of it

      export interface PropertyLike {
        name: string
        valueDescription?: string
      }

@jfbrennan
Copy link

jfbrennan commented Oct 17, 2022

I came here to start building a manifest for my custom elements and it seems support for documenting attributes is very limited. FWIW, I was expecting to see something like this (assume a Tooltip component):

"attributes": [
            {
              "name": "open",
              "type": "boolean",
              "description": "If present, the tooltip will be shown."
            },
            {
              "name": "pos",
              "type": "enumerated",
              "description": "Sets the position of the tooltip.",
              "values": ["top", "right", "bottom", "left"],
              "default": "top"
            }
          ]

Hopefully this shines some more light on developers' needs. Looking forward to seeing this project progress and gain widespread adoption!

@jogibear9988
Copy link
Contributor Author

@jfbrennan
if you have a initial version of your manifest, you could test your package in my designer: https://github.com/node-projects/web-component-designer it should be able to load your package via the manifest

@prantlf
Copy link

prantlf commented Apr 5, 2023

An inspiration from Custom Data for HTML Language Service:

  • Type is separate from, the enum values.
  • The enum values are specified by a reference to a separate valueSet object to avoid duplication, if the same values are used for another element's attribute.
{
  "tags": [
    {
      "name": "holy-grail",
      "description": "Provides an extended \"Holy Grail\" layout as a web component.",
      "attributes": [
        {
          "name": "size",
          "description": "`size` {`string`} - \n\nProperty: size\n\nDefault: `null`\n\nDisables the responsive behaviour by forcing the width to be detected as \"small\", \"medium\" or \"large\".",
          "valueSet": "sizes",
          "references": [
            {
              "name": "Documentation",
              "url": "https://github.com/prantlf/holy-grail-layout#attributes"
            }
          ]
        }
      ]
    }
  ],
  "valueSets": [
    {
      "name": "sizes",
      "values": [
        {
          "name": "small",
          "description": "Force the responsive behaviour as if the screen width were \"small\" - narrower than 480px."
        },
        {
          "name": "medium",
          "description": "Force the responsive behaviour as if the screen width were \"medium\" - wider than 479px and narrower than 768px."
        },
        {
          "name": "large",
          "description": "Force the responsive behaviour as if the screen width were \"large\" - wider than 767px."
        }
      ]
    }
  ]
}

@next-juanantoniogomez
Copy link

What about this issue? Thanks!

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

No branches or pull requests

6 participants