MudButton: Add ability to set custom rel attribute content

[TehGM]

Description

Allow MudButton, MudFab and MudIconButton to have rel="nofollow" as well as rel="noopener nofollow". allow setting custom content for rel attribute.

How Has This Been Tested?

Added new unit tests + tested output HTML using docs page

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.
• I've added relevant tests.

[xC0dex]

Hey @TehGM,

I just reviewed your PR and would like to give you some feedback 🎉
You used a code block inside a Razor file. You should consider using the code behind class file of the Razor component instead to separate presentation code from the actual logic of the component.
It looks like the code is the same in 3 components, so maybe this can be abstracted and reused.
The current implementation allocates memory for a list on each render process. Just by using normal string concatenation, the performance can be significantly improved:

Method	Mean	Error	StdDev	Ratio	Allocated	Alloc Ratio
GetRelWithList	44.324 ns	0.3725 ns	0.3302 ns	1.00	128 B	1.00
GetRelWithConcat	9.868 ns	0.1774 ns	0.1660 ns	0.22	56 B	0.44

In addition, I noticed that there is a change in the project file of the MudBlazor.Docs. I'm uncertain if this is required.

Other than that, nice addition 🥳

[TehGM]

@xC0dex Hey, thanks for the response.
The code duplication, I actually had in mind to fix it (I am a person that really dislikes duplicating code), but honestly forgot. Thanks for reminding!
Re string concat - yeah, I had a feeling that a List might be overkill and will likely come up in the review, but I did no performance test. I used it purely to make it easy to separate with a space, so now I opted for string concat + Trim. I also noticed there's a ValueBuilder utility class in the codebase, however decided to go with simple conditional concat for now - let me know if you think I should change it.
And the csproj change was an oversight on my end. :D

Pushed an update now, let me know if there's any more feedback.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (e19ac6f) 90.87% compared to head (4e6853d) 90.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #6301      +/-   ##
==========================================
+ Coverage   90.87%   90.88%   +0.01%     
==========================================
  Files         400      400              
  Lines       14486    14489       +3     
==========================================
+ Hits        13164    13169       +5     
+ Misses       1322     1320       -2     

Impacted Files	Coverage Δ
src/MudBlazor/Components/Button/MudButton.razor	100.00% <ø> (ø)
src/MudBlazor/Components/Button/MudFab.razor	100.00% <ø> (ø)
...rc/MudBlazor/Components/Button/MudIconButton.razor	100.00% <ø> (ø)
src/MudBlazor/Base/MudBaseButton.cs	88.46% <100.00%> (+1.50%)	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TehGM]

@xC0dex pushed a change that resolved some small conflicts with the new stuff on dev branch.

[ScarletKuro]

@henon and @Mr-Technician, I'm going to add you both for review. However, I'm unsure about the design. "rel" can have many more values, and are we going to add a bool property for each one? What if someone requests "noreferrer" in the "rel" attribute (as example)?

[Mr-Technician]

I don't think we should implement a boolean in this manner. Users can already add rel="" links and they will be applied to the underlying button with the UserAttributes.

[TehGM]

I don't think we should implement a boolean in this manner. Users can already add rel="" links and they will be applied to the underlying button with the UserAttributes.

@Mr-Technician I just tested this, and this appears to not work. Razor code:

<MudButton Href="@this.Clan.WebsiteURL" Target="_blank" Variant="Variant.Text"Color="Color.Tertiary" StartIcon="@AppIcons.Buttons.Website" UserAttributes="_linkButtonAttributes">
    Website
</MudButton>

The dictionary:

private static readonly Dictionary<string, object> _linkButtonAttributes = new Dictionary<string, object>()
{
    { "rel", "nofollow" }
};

Because of target being _blank for the button, this is the resulting HTML output (trimmed child nodes for brevity):

<a type="button" href="https://bloodxtract.com" target="_blank" rel="noopener" class="mud-button-root mud-button mud-button-text mud-button-text-tertiary mud-button-text-size-medium mud-ripple" _bl_62="">...</a>

I also tested removing Target attribute and providing it in the dictionary, but this results in no rel nor target being rendered at all - am I missing something?

[Mr-Technician]

I'm not sure @TehGM. I unfortunately only have my M1 Mac at the moment and am unable to build MudBlazor to debug this. You could also try setting rel=noopener directly on the MudButton but I doubt that will work if manually adding a dictionary does not.

[TehGM]

I'm not sure @TehGM. I unfortunately only have my M1 Mac at the moment and am unable to build MudBlazor to debug this. You could also try setting rel=noopener directly on the MudButton but I doubt that will work if manually adding a dictionary does not.

@Mr-Technician Yeah, as expected, that doesn't work.

I agree that a multitude of booleans is less than ideal, and something like enum flags (or something else, enum flags is just the first idea) could be cleaner. However it likely would make it a breaking change, unless some tricks are implemented to handle it.

And I'd personally argue that possibility of adding nofollow, while might not be absolutely critical, is pretty important when it comes to UGC.

[Mr-Technician]

And I'd personally argue that possibility of adding nofollow, while might not be absolutely critical, is pretty important when it comes to UGC.

I agree, but the best method may be attribute splatting. I will look into the issue with UserAttributes when I can.

[ScarletKuro]

@Mr-Technician why can't we just do something simple as that?

[Parameter]
[Category(CategoryTypes.Button.ClickAction)]
public string[] Rels { get; set; } = Array.Empty<string>();

protected virtual string GetRels()
{
	IEnumerable<string> values = Rels;
	if (Target)
	{
		values = new[] { "_blank" }.Concat(Rels);
	}
	return string.Join(" ", values.Select(x=> x.Trim()));
}

[Mr-Technician]

@Mr-Technician why can't we just do something simple as that?

I mean sure, you could, but attribute splatting is cleaner.

[TehGM]

I am not sure if it's that much cleaner actually. Having to declare a dictionary, while isn't end of the world, isn't the most hands-off approach. Array is slightly cleaner in that regard, and boolean even more. But of course, it's a balancing act to decide which is better in the end.

[Mr-Technician]

I am not sure if it's that much cleaner actually. Having to declare a dictionary, while isn't end of the world, isn't the most hands-off approach. Array is slightly cleaner in that regard, and boolean even more. But of course, it's a balancing act to decide which is better in the end.

You don't need to declare a dictionary. What you should be able to do (but can't due to some bug with MudButton) is this:

<MudButton rel="noopener" />

[Mr-Technician]

Ah, here is our trouble:

rel="@(Target=="_blank"?"noopener":null)"

rel is defined automatically if Target is _blank. So it is possible to set noopener but it's not flexible. I don't mind it being automatic, but people can't set it manually or add noreferrer as well, for example. A few things we could do:

1. Do nothing and instruct people to use Target=_blank (bad)
2. Remove the rel code in line 13, but modify the user dictionary (if rel == null) with the same logic for backwards compatibility (better)
3. Remove the rel code entirely in v7 (best)

Probably we should do 2 for now and add a comment in the summary and docs that it will be removed in v7. Unfortunately we aren't actually changing the API so we can't [Obsolete] it.

Thoughts @henon?

[henon]

I would opt for a string parameter Rel similar to the parameters Href and Target. If not set we can still do the automatic application of noopener and document it if that is ok. Or are there concerns against this automatism?

[Mr-Technician]

I like this option @henon.

What do you think? @TehGM

[TehGM]

@Mr-Technician I can investigate it after the weekend.

[TehGM]

Hey @Mr-Technician @henon
I am now back from holidays and I implemented the change as suggested, and I do agree it's a workable approach. Docs and tests are updated to reflect the addition correctly. Also updating the PR title in a moment.

It's pushed and ready for review now.

[Mr-Technician]

Thanks, I will look at this tonight and will merge if I don't have any comments.