havit / Havit.Blazor

Free Bootstrap 5 components for ASP.NET Blazor + optional enterprise-level stack for Blazor development (gRPC code-first, layered architecture, localization, auth, ...)
https://havit.blazor.eu
MIT License
461 stars 61 forks source link

Please, remove hardcoded icons! #97

Closed HybridSolutions closed 2 years ago

HybridSolutions commented 2 years ago

I understand the beauty of hardcoded icons but in my humble opinion this is not a good approach for expansibility. Everytime an icon is added you have to recompile the entire Havit.Blazor solution for those icons to be generated from the json file. What if an icon changes or is removed one day? All projects updating to a newer version of Havit.Blazor will fail forcing changes to the code.

protected override void BuildRenderTree(RenderTreeBuilder builder)
{
    // no base call

    builder.OpenElement(0, "i");
    builder.AddAttribute(1, "class", CssClassHelper.Combine("hx-icon", "bi-" + Icon.Name, CssClass));
    builder.AddMultipleAttributes(2, AdditionalAttributes);

    builder.CloseElement(); // i
}

Please reconsider this and allow normal CSS class strings to be applied when necessary. Let the developers decide what tools to use and when. Changing the approach will also allow to use other icons like FontAwesome.

hakenr commented 2 years ago

Our current design allows you/us to add any additional icons in your project or in the library.

  1. Create a new component, let's say MyFontAwesomeIconComponent, which will be able to render your icons. Something similar to HxBootstrapIcon: https://github.com/havit/Havit.Blazor/blob/72a3916c2863d10e91b1299baf19845643846839/Havit.Blazor.Components.Web.Bootstrap/Icons/HxBootstrapIcon.cs#L6-L34

  2. Create a new class derived from IconBase, let's say FontAwesomeIcon, the same way our BootstrapIcon does. https://github.com/havit/Havit.Blazor/blob/72a3916c2863d10e91b1299baf19845643846839/Havit.Blazor.Components.Web.Bootstrap/Icons/BootstrapIcon.cs#L14-L34 You just have to forward the rendering to a rendering component there by overriding the RendererComponentType to point to your MyFontAwesomeIconComponent.

Now, you can use the FontAwesomeIcon everywhere in our library where there is IconBase-type parameter allowing you to set custom icon.

  1. [OPTIONAL] You can create static properties in the FontAwesomeIcon class (or anywhere else) where you will create named icons to not having to type new FontAwesomeIcon() { Xyz="...", ..." } for every single use.
    public static FontAwesomeIcon SomeIcon => new FontAwesomeIcon() { CssClass="fa-some-icon", ... }

What if an icon changes or is removed one day? All projects updating to a newer version of Havit.Blazor will fail forcing changes to the code.

Yes, that's a breaking change, that's how it goes. Our library is wrapping the underlying Bootstrap and Bootstrap Icons and we do not intend to fight with breaking changed they decide to make (and we hope there won't be much). No matter if they decide to remove a whole component, or to remove/change an icon, we will most likely mirror such change in our library resulting in a breaking change for our users.

hakenr commented 2 years ago

TODO: New documentation topic - Creating a custom icon-set.

HybridSolutions commented 2 years ago

Thanks for the detailed explanation but still don't like the hardcoded icon solution. Having to create a class and component to hardcode hundreds of CSS class names is not a good thing. Design in applications changes often and so does graphic elements and fonts. Having to map those kind of elements when it's so easy to just use "fa-some-icon", is an insane amount of work and you know it since you are using a source generator for that task.

I don't know how to use a source generator and I doubt most developers do. Also SourceGenerator is somehow problematic with .NET 6 and VS. You have to target the class library to .NET standard 2.0 and even then there's known problems with VS 2022. After cloning your repo and opening it in VS I couldn't manage to have the bootstrap icons generated and compile the solution because of that.

The optional way is to hand code each icon of a library that has hundreds of icons or force to handpick those that will be required by the designer and hoping he won't change his mind in the future.

Why are you using a component as a parameter? Couldn't you render the <i> tag and just assign a CSS class specified by a string parameter in the Grid component?

Sorry if I seem a little harsh but have to confess this design choice for such a good project is driving me crazy.

hakenr commented 2 years ago

Just added instructions and samples for creating custom icons to documentation: https://havit.blazor.eu/components/HxIcon#custom-icons

In fact you don't have to create properties for all the individual icons if you don't want (something we use source generator for), you can create the instances inline:

<HxIcon Icon="new MyCustomIcon("fa-some-icon")" />
<HxButton Icon="new MyCustomIcon("fa-some-icon")" ... />