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

Bundle size #5

Open
miniBill opened this issue Aug 21, 2024 · 5 comments · May be fixed by #6
Open

Bundle size #5

miniBill opened this issue Aug 21, 2024 · 5 comments · May be fixed by #6
Assignees
Labels
bug Something isn't working

Comments

@miniBill
Copy link
Contributor

While the current API feels very nice, it has the disadvantage that live code inclusion only works at the icon level, and not at the style level.

So if I use - say - Phosphor.cloud, that includes all styles in the final JS bundle.

I wonder if it would be better to have something like https://package.elm-lang.org/packages/lattyware/elm-fontawesome/latest/FontAwesome-Solid, with one module per style.

I'm up for writing a PR for this, if you want. If so, I'd be tempted to use elm-codegen instead of the current JS.

Let me know:

  1. what do you think of this issue
  2. if you're interested in a PR, and if so
    2.1. if you would be fine with a PR using elm-codegen
@miniBill miniBill added the bug Something isn't working label Aug 21, 2024
@rektdeckard
Copy link
Member

rektdeckard commented Aug 21, 2024

I personally tend to use 2-3 weights when building applications: Regular and Fill weights to denote statefulness (active/inactive) of icons at a larger size, and often using Bold for small indicator icons. This works well for my use case, and I like the comfort of how it is set up now.

I'm not totally against your proposal, but I'd want to hear it from more people before agreeing to a big breaking change like this, even in Elm where breaking changes are pretty easy. How many icons are you using in your application that the extra cost per icon is too much? Can you share some numbers? Would love to see real-world savings, as I not sure this is the source of any real issue.

@rektdeckard
Copy link
Member

rektdeckard commented Aug 21, 2024

To your other point though, I'd be happy to move the codegen to elm-codegen. This was initially set up at the same time as some of our other JS libs and it made sense at the time to copy and tweak the stuff I had already written.

@miniBill
Copy link
Contributor Author

I see how if you regularly change the weight then the current API is nicer.

I wonder if you could simply have the best of both worlds by having the per-weight modules in addition to the current single module.

@rektdeckard
Copy link
Member

I'm cool with that, if you'd like to make the change! Are you thinking to expose as a separate or child module so there aren't namespace collisions?

@miniBill
Copy link
Contributor Author

I'd call them Phosphor.Regular, Phosphor.Duotone, ...

@miniBill miniBill linked a pull request Aug 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants