carloskiki / leptos-icons

An icon library for the leptos web framework
MIT License
95 stars 22 forks source link

Inability to name arbitrary icon #13

Closed lpotthast closed 1 year ago

lpotthast commented 1 year ago

It seems that there is not generalized icon enum anymore. That makes it hard to "name" or ''reference'' a single but arbitrary icon. For the lib itself and it's Icon component, taking anything convertable into icondata_core::IconData is fine, but if other libraries want to reference an arbitrary icon, this design forces them to use icondata_core::IconData as well, which currently provides no functionality, does not derive common traits and can not provide information about what icon that data actually represents. It is kind of unsuitable to be send around in code or over network or into storage. An enum would be more appropriate. I would suggest, if it does not induce a large penalty to final (wasm) binary size, to re-add the old Icon enum. This could then derive

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)]

and should probably also derive serde::Serialize and serde::Deserialize (hidden behind a serde feature flag) so that users can easily send around and store any chosen icon without introducing own wrapper types. The Icon component can still take anything being Into<IconData>.

carloskiki commented 1 year ago

Okay, I get your use case.

One thing I think we could do is reexport the IconData struct from leptos_icons.

For the issues regarding the traits, I think the functionality is still there. Let's say one wants to create a component that takes any arbitrary icon, and has to implement Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy:

#[component]
fn MyComponent<T>(cx: Scope,
    icon: T
) -> impl IntoView
where
    T: Debug + PartialEq + Eq + PartialOrd + Ord + Hash + Clone + Copy + Into<IconData>,
{
    todo!()
}

Prove me if I'm wrong, but if leptos_icons simply reexports IconData, this type of behaviour would work. (every packages' icon enum still implement all of these traits, plus serde traits and strum traits.)

lpotthast commented 1 year ago

I still think that it would be overall much easier for users if an Icon enum would be present, encapsulating the enum types of the individual icon libraries.

I already tried depending on the current icondata_core version and using IconData anywhere I previously used the Icon enum.

Yes, one could work with the IconData type directly and depend on the user providing the IconData directly by specifying an specific enum variant, but what if (in your example) your icon is optional? The need for the generic type limits you there, as leptos in general doesn't play nice with optional generic props if omitted. There are ways around that, but nowhere near as convenient as just taking Option<Icon>.

Making more components generic could also impact binary size in the end. I do not now if this library should force such a design. The enum was like 40 LOC.

What if you want to encapsulate multiple icons in your own struct and use that as a prop? Youd would basically need to wrap all members in a newtype and probably write an Icon enum yourself.

If you use the icons in reactive parts of your component, you will move them around, wanting to trivially copy them and so on. Yes, IconData is trivially copyable (or could be). For now. It just doesn't seem right to me to juggle around the whole icon data struct.

Especially when enums like BsIcon exists, which would allow a user to name any Bs icon, users are left in the dark about how "any" icon (Bs, Fa, ...) should be referenced.

Storing a representative of "any" icon is also not possible when only the IconData struct is exposed and expected to be used. I wouldn't want to serialize that and send it somewhere or store it in local storage.

In fact, any user will probably never need anything from that IconData struct at all. Only the library needs it to display the icon.

I always thought about the IconData struct as an internal structure to this icon library which probably shouldn't be exposed if not required.

carloskiki commented 1 year ago

Yes okay, you totally sold me on this. I will open a PR soon to reimplement this feature.

carloskiki commented 1 year ago

The feature is now reimplemented!