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

Enable the type to be an enumerator instead of a type. #25

Closed
felipeplets opened this issue Mar 23, 2020 · 10 comments · Fixed by #36
Closed

Enable the type to be an enumerator instead of a type. #25

felipeplets opened this issue Mar 23, 2020 · 10 comments · Fixed by #36

Comments

@felipeplets
Copy link
Collaborator

I've been using the library since version 2 and for my use case the best would be that type can be an enumerator instead so that users have easily idea of all the icon options available in the library.

@kreuzerk
Copy link
Owner

We can do that 👍Would be string enum because I think if you use the icons with a registry it's better to have a string as runtime representation than a number.

Generally, you should already get a list of all available icons by using the type. Usually, your IDEA can display them and you would see a list of all available icons. So if for example, your component accepts an icon of myIcon as @Input, the IDEA should give you a list of available icons as a hint.

Just out of curiosity, what is your use case?

@felipeplets
Copy link
Collaborator Author

Yes, I use IntelliJ and VSCode. My VSCode does not autocomplete it but IntelliJ does, although is not quite the same experience as with an enum.

But my main use case is that I have a components library using Storybook, and I can easily provide the list of available icons through a dropdown to the end user (UI/UX) team by setting a select knob with the type of the enumerator.

I'm half way with a PR to the project with the enum implementation. My idea was to implement a variable "enumType": true.

@kreuzerk
Copy link
Owner

Wow. Sounds amazing. Looking forward to the PR! 👍
Yes, a flag enumType which defaults to false sounds good.

@akehir
Copy link
Contributor

akehir commented Mar 25, 2020

I second that. Enum instead of type! It is also possible to iterate through an enum, since it doesn't get "lost" from TS -> JS compilation. Therefore creating a demo page showing off all icons is much easier.

@kreuzerk
Copy link
Owner

kreuzerk commented Apr 1, 2020

I think we should support both. Maybe some background why it is currently a type. Most often the icons are used with a component and a registry. The usage for an end consumers would be the following:

<my-icon name="foo"></my-icon>

In such a scenario using a type has the following advantage.

  1. Usually ideas are able to give you a preview of all available icon types when you want to pass the icon name over an @Input() to your icon component. With an enum, it would only show you the enum name and not the values.

  2. With a type you can simply pass down the value as a string. Like illustrated above. With an enum on the other hand you have to first assign the enum value to a public variable to make it available in your template.

template: `<my-icon name="myCustomIcons.foo"></my-icon>`
// ...
export class MyComponent {
   myCustomIcons = myCustomIconsEnum;
}

But I also see the point where it is easier to display all the available icons.

@felipeplets
Copy link
Collaborator Author

I understand @kreuzerk.

I'm currently finishing this implementation and thus I was thinking that we can add two options:

1 - -e --enumName to name the enumerator
2 - -de --disableEnum to disable it (in case the default behaviour is to generate it) or -de --enableEnum (in case the default behaviour is to not generate it)

My only question is what do you believe is the best as the default option?

@akehir
Copy link
Contributor

akehir commented Apr 5, 2020

It's also possible to export both an quasi enum and a type, in this way:

export type DinosaurIcon = 'tyrannosaurusIcon' | 'stegosaurusIcon';
export const DinosaurIcon = {
    tyrannosaurusIcon: 'tyrannosaurusIcon' as DinosaurIcon,
    stegosaurusIcon: 'stegosaurusIcon' as DinosaurIcon
};

Then you can pass in either a string, as currently, or use the DinosaurIcon.tyrannosaurusIcon.

And easily map it to an array, like so:

export function enumToStringArray<T>(_enum: Object): T[] {
  return Object.keys(_enum).map(k => _enum[k]);
}

const iconArray = [
    ...enumToStringArray<DinosaurIcon>(DinosaurIcon),
];

@felipeplets
Copy link
Collaborator Author

@akehir I had implemented it as an enum but for my use case the object works too and we can use the same name so would be a much simpler implementation because no option would be needed in this case. This is the PR #36 (it has more code because it depends on another PR I raised earlier today #35)

@kreuzerk
Copy link
Owner

kreuzerk commented Apr 5, 2020

I personally would prefer to generate types by default. Then I would also offer the possibility to generate only the const or const and type like suggested by @akehir. I would not generate both by default.

I imagine some options like this

generateTypes: boolean (default: true);
generateTypeObject:boolean (default: false);

@kreuzerk kreuzerk linked a pull request Apr 12, 2020 that will close this issue
@kreuzerk
Copy link
Owner

Fixed with version 4.2.0. Thanks to everybody involved in the discussion and special thank to @felipeplets for the awesome PR.

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 a pull request may close this issue.

3 participants