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

Update to HeroIcons v2.1.5 #39

Merged
merged 2 commits into from
Aug 20, 2024
Merged

Update to HeroIcons v2.1.5 #39

merged 2 commits into from
Aug 20, 2024

Conversation

goncalotomas
Copy link
Contributor

A few new icons have been added since 2.1.1. This PR updates the project dependencies and moves the current HeroIcons version to 2.1.5. Open to feedback of course :)

@@ -112,7 +112,7 @@ defmodule Heroicons do
"""
end

<%= for {func, [outline, solid, mini, micro]} = icon when not is_nil(micro) <- @icons do %>
<%= for {func, [outline, solid, mini, micro]} = _icon when not is_nil(micro) <- @icons do %>
Copy link
Contributor Author

@goncalotomas goncalotomas Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both changes in this file fix a compile warning

@greven
Copy link
Contributor

greven commented Aug 13, 2024

Was about to submit a PR for this, I know that the default in Phoenix now is to use Tailwind and the icons with CSS, but as someone that don't use Tailwind in my current project, this package is still very useful. :)

@mveytsman can we get this merged, please?

@mveytsman mveytsman self-requested a review August 16, 2024 21:45
@mveytsman
Copy link
Owner

@goncalotomas thank you!

Do you mind fixing the failing tests?

@goncalotomas
Copy link
Contributor Author

goncalotomas commented Aug 20, 2024

These test failures are pretty interesting in that they relate to the order of attributes after being rendered out, e.g. it seems that in OTP versions before 26 the svg will render out to:

<svg xmlns=\"http://www.w3.org/2000/svg\" aria-hidden=\"true\" fill=\"none\" stroke=\"currentColor\" stroke-width=\"1.5\" viewBox=\"0 0 24 24\">
[...]
</svg>

and after OTP26 it renders out to this:

"<svg xmlns=\"http://www.w3.org/2000/svg\" aria-hidden=\"true\" fill=\"none\" viewBox=\"0 0 24 24\" stroke=\"currentColor\" stroke-width=\"1.5\">
[...]
</svg>"

The main difference being that the class and viewBox attributes get listed in a different order, and I suspect it's related to map ordering changes in OTP26 / Elixir 1.14 (more info). I was able to replicate the error by going back to OTP24.2 and Elixir 1.14.5: using the same versions as CI made the tests fail, but switching to OTP26 and Elixir 1.16 these tests pass.

TL;DR If we make the tests work for current OTP/Elixir, they break in the older versions and vice versa.

There's a decision to make, and I see three ways forward:

  • We stay aware of this for further changes to the library but decide to just keep things the way they are for now
  • We improve the tests so that they don't particularly care about the ordering of attributes
  • We patch the tests so they work with current OTP/Elixir and stop testing for < OTP26

All of these have different trade-offs, so I'll leave it up to @mveytsman to decide :)
Also, we don't have to decide now. I've pushed a commit that solves the issue in CI, but any future updates will most likely run into this issue again.

Have a great day!

@mveytsman
Copy link
Owner

@goncalotomas thank you so much for figuring this out and writing it up!

I think the right move is to keep this in mind and fix it next release, I'll see about improving the tests!

@mveytsman mveytsman merged commit e9f86ff into mveytsman:main Aug 20, 2024
1 check passed
@mveytsman
Copy link
Owner

Published!

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 this pull request may close these issues.

3 participants