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

Increase icon flexibility #518

Merged
merged 16 commits into from
Dec 3, 2024

Conversation

keatz55
Copy link
Contributor

@keatz55 keatz55 commented Nov 29, 2024

Color mode icons don't render unless using paid FontAwesome plan. Also, the phoenix famework has since changed how it implements heroicons. As a result, this PR makes the following updates:

  • Adds new local_icon component for custom localized icons
    • allows for latest phoenix framework hero icon implementation
    • makes icons future proof by allowing users to use local implementations
  • adds support for color_mode_icons config override
    • defaults to current fa icons
    • allows users to swap out color mode icons w/ local and/or hero provider icons

sourcery-ai[bot]

This comment was marked as duplicate.

@keatz55
Copy link
Contributor Author

keatz55 commented Nov 29, 2024

I have to step out, but will ensure works as expected for sidebar/page-header icons a bit later.

@cblavier
Copy link
Contributor

cblavier commented Nov 30, 2024

All looks great, thanks! ❤️
I will review it this Monday

Copy link
Contributor

@cblavier cblavier left a comment

Choose a reason for hiding this comment

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

Thanks for your awesome work 🙏

I added some comments and noticed a few other quirks:

  • validation_helpers.ex raises when declaring a local icon in a story
  • we need to relax heroicons dependency to allow users to pick the heroicons elixir package or the TailwindCSS one
  • when declaring a local icon with {:local, "hero-check-badge", :micro} Tailwind JIT won't pick the hero-check-badge-micro variant. This is working with {:local, "hero-check-badge-micro"}. Should we drop the style option for local_icons?

Do you want to continue, or do you prefer I take over?

@phenixdigital phenixdigital deleted a comment from sourcery-ai bot Dec 2, 2024
@keatz55
Copy link
Contributor Author

keatz55 commented Dec 2, 2024

Thanks for your awesome work 🙏

I added some comments and noticed a few other quirks:

  • validation_helpers.ex raises when declaring a local icon in a story
  • we need to relax heroicons dependency to allow users to pick the heroicons elixir package or the TailwindCSS one
  • when declaring a local icon with {:local, "hero-check-badge", :micro} Tailwind JIT won't pick the hero-check-badge-micro variant. This is working with {:local, "hero-check-badge-micro"}. Should we drop the style option for local_icons?

Do you want to continue, or do you prefer I take over?

@cblavier I'm flexible. It's your lib so I understand if you have a desired path forward for relaxing heroicons, etc. I'll defer to you, but I can cut the requested changes later today if desired.

@cblavier
Copy link
Contributor

cblavier commented Dec 2, 2024

Thank you!
You can proceed with the changes, and I'll do a final round of polishing before we ship 🚀

@keatz55
Copy link
Contributor Author

keatz55 commented Dec 3, 2024

@cblavier Just pushed the following requested changes:

  • fix validation_helpers.ex errors
  • remove :local icon style attr to prevent false sense of security around tw jit comp
  • doc example improvement/fixes
  • fix color mode icon fa_plan={@fa_plan} attr in header
  • phoenix_storybooke.ex Macro.escape(opts) simplification

Let me know if I missed anything

@cblavier cblavier force-pushed the jw/increase-icon-flexibility branch from fc65fb6 to c04f66b Compare December 3, 2024 10:56
@cblavier cblavier force-pushed the jw/increase-icon-flexibility branch from 6542185 to 3c7cbff Compare December 3, 2024 11:03
@cblavier cblavier force-pushed the jw/increase-icon-flexibility branch from 3c7cbff to 2afa130 Compare December 3, 2024 11:34
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.90%. Comparing base (417c7cb) to head (50b74c6).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #518      +/-   ##
==========================================
- Coverage   95.91%   95.90%   -0.01%     
==========================================
  Files          38       38              
  Lines        2054     2052       -2     
==========================================
- Hits         1970     1968       -2     
  Misses         84       84              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cblavier cblavier force-pushed the jw/increase-icon-flexibility branch from 1c842ff to d8d3b38 Compare December 3, 2024 14:07
@cblavier cblavier force-pushed the jw/increase-icon-flexibility branch from d8d3b38 to 50b74c6 Compare December 3, 2024 14:09
@cblavier cblavier merged commit 43f3e64 into phenixdigital:main Dec 3, 2024
3 checks passed
@cblavier
Copy link
Contributor

cblavier commented Dec 3, 2024

🎉 🔥 ❤️

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.

2 participants