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

Inability to name arbitrary icon #13

Closed
lpotthast opened this issue Apr 29, 2023 · 4 comments
Closed

Inability to name arbitrary icon #13

lpotthast opened this issue Apr 29, 2023 · 4 comments

Comments

@lpotthast
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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

@carloskiki
Copy link
Owner

The feature is now reimplemented!

carloskiki pushed a commit that referenced this issue Oct 8, 2023
Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
carloskiki added a commit that referenced this issue Oct 8, 2023
* Add one codecov

* Merge another codecov

* Merge another codecov

* Merge another codecov

* Merge another codecov

* Place codecov config under .github

* Add (only) ASAN workflow

* Add first coverage workflow

* Merge another coverage.yml

* Merge another coverage.yml

* Add first features workflow

* Merge another features workflow

* Merge another features workflow

* Merge another features workflow

* Add (only) loom workflow

* Add (only) LSAN workflow

* Add first minial workflow

* Add (only) miri workflow

* Add first msrv workflow

* Merge another msrv workflow

* Merge another msrv workflow

* Merge another msrv workflow

* Add (only) no-std workflow

* Add first os-check workflow

* Merge another os-check workflow

* Add first style workflow

* Merge another style workflow

* Merge another style workflow

* Add first test workflow

* Merge another test workflow

* Merge another test workflow

* Merge another test workflow

* Make everything use checkout@v3

* Standardize on 'main' as branch name

* Missed a submodule checkout

* Add TODOs from twitter thread

* Practice what you preach

* mv github .github

This should make it possible to have rust-ci-conf as a remote you merge
from.

* Merge safety workflows

* Catch upcoming deprecations

* More concise name for scheduled jobs

* Allow examples and binaries to require features

* Use dependabot, but only for major versions

* ignore is a list

* Notify if actions themselves are outdated

* Bump codecov/codecov-action from 2 to 3

Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 2 to 3.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@v2...v3)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

* Move to maintained rust installer

See actions-rs/toolchain#216

* Fix install message for msrv

* Get rid of most actions-rs bits

Given that that project is unmaintained.

actions-rs/toolchain#216

* Minimal token permissions

See tokio-rs/tokio#5072

* Remove -Zmiri-tag-raw-pointers as it's now default

* Unbreak cargo hack for non-libraries (#4)

* Add action to run doctest. (#3)

`cargo test --all-features` does not run doc-tests. For more information
see rust-lang/cargo#6669.

* chore: automatically cancel superseded Actions runs (#5)

* [sanity] More robust injection of opt-level 1 (#9)

Fixes #8

* Quote MSRV version to avoid float parsing (#11)

Put 1.70 in there (for instance if you want to pin against OnceLock stabilizing) and it will actually test 1.7 as it appears github auto converts this to a float?

Putting in quotes seems to do the right thing here

* Install Openssl for Windows (#12)

* Don't install OpenSSL on Windows by default

* Bump actions/checkout from 3 to 4 (#13)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Jon Gjengset <jon@thesquareplanet.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tudyx <56633664+Tudyx@users.noreply.github.com>
Co-authored-by: Burkhard Mittelbach <wasabi37a@googlemail.com>
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
Co-authored-by: James Chacon <chacon.james@gmail.com>
Co-authored-by: Rod Elias <rodiney@gmail.com>
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

2 participants