microsoft / fluentui-blazor

Microsoft Fluent UI Blazor components library. For use with ASP.NET Core Blazor applications
https://www.fluentui-blazor.net
MIT License
3.8k stars 368 forks source link

fix: `FluentButton` has `Icon` in wrong `slot` if `ChildContent`, when `null`, is left in markup #2863

Open Eagle3386 opened 2 days ago

Eagle3386 commented 2 days ago

šŸ› Bug Report

Using FluentButton to be, depending on a given parameter, either in "ico-only mode" or with a label next to the icon, the icon is added to the wrong slot.

šŸ’» Repro or Code Sample

Create a razor component with this markup:

@* NOTE: Formatted for readability, bug still occurs if written as one-liner! *@
<FluentButton IconStart="new Microsoft.FluentUI.AspNetCore.Components.Icons.Regular.Size24.SignOut()">
  @ChildContent
</FluentButton>

šŸ¤” Expected Behavior

Button: Image Code: Image

šŸ˜Æ Current Behavior

Button: Image Code: Image

šŸ’ Possible Solution

If ChildContent is null, i.e., "not there", Icon should act accordingly & move into the default slot instead of start - this is even the default behavior of Razor, not only Blazor.

For now, I have to violate DRY by doing this:

@if (ChildContent is not null)
{
  <FluentButton IconStart="Icon" Id="@Id">@ChildContent</FluentButton>
}
else
{
  @* NOTE: In reality, I simplify the line below into an empty / self-closing / standalone tag *@
  <FluentButton IconStart="Icon" Id="@Id"></FluentButton>
}

šŸ”¦ Context

Implementation of a button component, toggleable to either "icon-only" or "icon with label" mode.

šŸŒ Your Environment

vnbaaij commented 2 days ago

We have the following code in the Button component for both IconStart and IconEnd:

<FluentIcon ... Slot="@(ChildContent != null ? "start" : null)" ...

("start" is "end" of course when using IconEnd)

Looking at https://www.fluentui-blazor.net/Button#withcontentandicon I see it is working as expected. If you want to have an icon only button with the icon being in the default slot, the ChildContent needs to be null. What are you missing?

Eagle3386 commented 2 days ago

I know, but my ChildContent is null, yet the icon isn't in the default slot, but in start. Here's the code of the relevant components:

Note: Icons is a using alias for Microsoft.FluentUI.AspNetCore.Components.Icons.Regular.Size24 due to (yet) no option for global icon style assignment, e.g., in Program.cs.

AppLoginButton.razor (used in 3 places throughout the migration phase)

<MyButton Appearance="Appearance" Icon="new Icons.SignOut()" Label="@(IsIconButton ? null : Label)"
          OnClick="LogInAsync" Style="@Style" Title="@(IsIconButton ? Label : null)" />
@code
{
  // Note: comments left out & properties turned into one-liners for brevity
  [Parameter] public Appearance Appearance { get; init; }
  [Parameter] public bool IsIconButton { get; init; }
  [Parameter] public string Label { get; init; } = "Log In";
  [Parameter] public string Style { get; init; }
}

MyButton.razor (used throughout the app instead of FluentButton)

<FluentButton Action="@Href" Appearance="Appearance" BackgroundColor="@Background" Class="@Class" Color="@Color"
              Disabled="IsDisabled" IconStart="Icon" Id="@Id" OnClick="OnClickAsync"
              Style="@Style.ToFullWidth(IsFullWidth)" Target="@Target" Type="Type">
  @(ChildContent is null ? Label : ChildContent)
</FluentButton>
@if (Title is not null)
{
  <MyToolTip Anchor="@Id" Text="@Title">@Title</HmdToolTip> @* Discussed in #2864 *@
}
@code
{
  // Note: comments left out & properties turned into one-liners for brevity
  [Parameter] public Appearance Appearance { get; set; } = Appearance.Accent;
  [Parameter] public RenderFragment ChildContent { get; set; }
  [Parameter] public string Background { get; set; }
  [Parameter] public string Class { get; set; }
  [Parameter] public string Color { get; set; }
  [Parameter] public string Href { get; set; }
  [Parameter] public Icon Icon { get; set; }
  [Parameter] public bool IsDisabled { get; set; }
  [Parameter] public bool IsFullWidth { get; set; }
  [Parameter] public string Label { get; set; }
  [Parameter] public EventCallback OnClick { get; set; }
  [Parameter] public EventCallback<MouseEventArgs> OnClickWithArgs { get; set; }
  [Parameter] public string Style { get; set; }
  [Parameter] public string Target { get; set; }
  [Parameter] public string Title { get; set; }
  [Parameter] public ButtonType Type { get; set; }

  private static readonly string[] ExternalSchemes = [Uri.UriSchemeFtps, Uri.UriSchemeHttps, Uri.UriSchemeMailto, "tel:"];

  private string Id { get; } = Identifier.NewId();

  protected override Task OnParametersSetAsync()
  {
    if (!string.IsNullOrEmpty(Href) && ExternalSchemes.Any(Href.StartsWith))
    {
      Target = "_blank";
    }

    if (Type == ButtonType.Reset)
    {
      Appearance =   Appearance == Appearance.Accent ? Appearance.Neutral : Appearance;
      Icon       ??= new Icons.Dismiss();
      Label      ??= GetLabelOrDefault("Stop");
    }
    else if (Type == ButtonType.Submit)
    {
      Icon  ??= new Icons.Save();
      Label ??= GetLabelOrDefault("Store");
    }

    return Task.WhenAll(InvokeAsync(StateHasChanged), base.OnParametersSetAsync());

    string GetLabelOrDefault(string @default) => ChildContent is null ? @default : null;
  }

  private Task OnClickAsync(MouseEventArgs args) =>
    OnClick.HasDelegate
      ? OnClickWithArgs.HasDelegate
        ? throw new InvalidOperationException($"Cannot set both {nameof(OnClick)} & {nameof(OnClickWithArgs)}.")
        : OnClick.InvokeAsync(args)
      : OnClickWithArgs.HasDelegate
        ? OnClickWithArgs.InvokeAsync(args)
        : null;

MyToolTip.razor

@inherits FluentTooltip
@code
{
  // Note: comments left out & properties turned into one-liners for brevity
  [Parameter] public bool AnchorValue { get; set; }
  [Parameter] public bool IsAnchorToggle { get; set; }
  [Parameter] public string Text { get; set; }
  [Parameter] public string TextSuffixFalse { get; init; } = "show";
  [Parameter] public string TextSuffixTrue { get; init; } = "hide";

  protected override async Task OnParametersSetAsync()
  {
    ChildContent ??= @<text>(IsAnchorToggle ? $"{Text} {(AnchorValue ? TextSuffixTrue : TextSuffixFalse)}" : Text)</text>;
    await Task.WhenAll(InvokeAsync(StateHasChanged), base.OnParametersSetAsync());
  }
}
vnbaaij commented 2 days ago

I know, but my ChildContent is null, yet the icon isn't in the default slot, but in start.

Sorry, but I think it is something on your side. If you look at the code that is generated with the sample I mentioned, you'll see the icon is placed in the default slot:

Image

How can't it be null in your code? If it is null it renders Label, right?

<FluentButton Action="@Href" Appearance="Appearance" BackgroundColor="@Background" Class="@Class" Color="@Color"
              Disabled="IsDisabled" IconStart="Icon" Id="@Id" OnClick="OnClickAsync"
              Style="@Style.ToFullWidth(IsFullWidth)" Target="@Target" Type="Type">
  @(ChildContent is null ? Label : ChildContent)
</FluentButton>

I understand this is an issue for you but we need to go with what we have in our code and examples and that shows it is showing the expected behavior. I don't mean this in a rude or unpleasant way, but we don't have a lot of time to work on this and we can't spend it on debugging your code.

Eagle3386 commented 2 days ago

Did you notice that Label might be null?

Besides, go ahead & change @(ChildContent is null ? Label : ChildContent) to simply @ChildContent - the outcome is the same, the icon is in the wrong slot. Because that's exactly how I narrowed it down prior to creating this issue.

It's a bug within the framework. I'm 99% sure.

vnbaaij commented 2 days ago

What happens if you try and run the code from our example without any changes?

I can't run your code as just copying and pasting it gives compilation errors (SignOut does not exist, etc, ToFullWidth does not exist...)

Eagle3386 commented 2 days ago

The example can't be taken into account as it doesn't resemble the hierarchy of 2 components (with a 3rd one for the tooltip - but even that could be left out for purpose of reproduction).

And while your point regarding ToFullWidth might be valid (though, also unnecessary for repo - i meant to provide you with all code, just to give you the bigger picture), the one regarding SignOut is not. I started the comment, containing the code snippet, with a note directly addressing that:

Note: Icons is a using alias for Microsoft.FluentUI.AspNetCore.Components.Icons.Regular.Size24 due to (yet) no option for global icon style assignment, e.g., in Program.cs.

Anyway, you can narrow it down to this:

MyComponent.razor:

<MyButton Icon="new Microsoft.FluentUI.AspNetCore.Components.Icons.Regular.Size24.SignOut()" />

MyButton.razor:

<FluentButton IconStart="Icon">@ChildContent</FluentButton>
@code
{
  [Parameter] public RenderFragment ChildContent { get; set; }
  [Parameter] public Icon Icon { get; set; }
}

Rendered HTML:

Image

vnbaaij commented 2 days ago

The example can't be taken into account as it doesn't resemble the hierarchy of 2 components (with a 3rd one for the tooltip - but even that could be left out for purpose of reproduction).

I'm sorry, but I think that is just BS. The example on the demosite that I linked to proves that a button works with icons exactly as it should. No, it does not resemble a hierarchy but we need to establish that the core button component does what it should do first. And, again, the example shows that. Then we (and by that I mean both you and us) need to find out why it is not working in your situation. It doesn't feel like you are telling us what you tried to solve the issue. It does feel like "here is my error and code, go fix it"

And while your point regarding ToFullWidth might be valid (though, also unnecessary for repo - i meant to provide you with all code, just to give you the bigger picture), the one regarding SignOut is not. I started the comment, containing the code snippet, with a note directly addressing that:

Note: Icons is a using alias for Microsoft.FluentUI.AspNetCore.Components.Icons.Regular.Size24 due to (yet) no option for global icon style assignment, e.g., in Program.cs.

Yes, you mentioned the icons indeed. But what matters to us is that the code you posted is not ready-to-run reproduction code. Do you expect us to copy 3 blocks, then paste it in 3 new files (which we need to create first), then go through all the icons mentioned, then remove or replace the ToFullWidth? Or do you expect us to compose our own reproduction based on your code sample?

I really hope you (and everyone else reading along) understand that that is not workable for us. It is not just your issue we need to do that for but also for all the other issues (and discussions) that get created. I've said it many times before but will repeat it here again: help us to help you!

Anyway, you can narrow it down to this:

MyComponent.razor:

MyButton.razor:

@ChildContent

@code { [Parameter] public RenderFragment ChildContent { get; set; } [Parameter] public Icon Icon { get; set; } }

That is much better and I assume I can just paste and run this but it does not answer my previous question: "What happens if you try and run the code from our example without any changes?"

Eagle3386 commented 2 days ago

First of all, I'd like to apologize. This obviously went into the wrong direction as your reflection, "here's my error & code, go fix it" is as close to what I intended as our beloved planet Earth to Alpha Centauri. šŸ˜‰

Next, I initially thought https://www.fluentui-blazor.net/issue-tester is sort of what https://try.mudblazor.net & the like are - online, quick-to-setup "mini-code editors", meant to create repo-samples by issue reporters like me. As a suggestion, I'd like to vote for such a "https://try.fluentui-blazor.net"-website.

Since that isn't/wasn't available & as I already said, the larger code snippets were meant for getting the bigger picture, not for re-creating it. šŸ˜… For that, I intended my OP's snippet - but yes, you'd have to create the outer/parent component yourself & yes, given it's probably mostly free time you're putting into this project, it might be a bit too much to ask.

Now, to answer your first question right away: yes, using the icon-only example (but removing the demo site's logger call) does render the icon as expected - though, the icon is set as IconEnd, not as IconStart. Noticing this, I adapted my code like this:

<FluentButton IconEnd="ChildContent is null && Label is null ? Icon : null"
              IconStart="ChildContent is null && Label is null ? null : Icon">@ChildContent</FluentButton>

& also like this:

<FluentButton IconEnd="Icon">@ChildContent</FluentButton>

with Icon being provided as Parameter - but neither attempt succeeded, except that the icon was placed into the end-slot instead of start like before at both attempts.

Lastly, I'm glad my last attempt at helping you helping me (as that's what I'm after right from the beginning - again, I apologize if that wasn't clear or if I sounded otherwise) went into the right direction. As I gave my answer to your last question earlier in this comment, there's nothing left I need to address, I guess.

Please, if there's anything else I can do: let me know. Right away. I'll try my best to help ASAP. Though, for the weekend, I'm unable to access my code base for further tests. šŸ™ˆšŸ˜…

vnbaaij commented 2 days ago

I guess we both are in need of the weekend... šŸ˜. Let's look at it again next week.

Eagle3386 commented 2 days ago

Now that's for sure! I already dreamed of FluentUI-Blazor code last night! šŸ™ˆ

But November 2nd will be "demo day" in front of hundreds of people for my employer & afterwards, if I'm still alive, all pressure will be gone. And my brain probably, too! šŸ¤Ŗ