MudListItem and MudMenuItem: Render as anchor with"Href" property (#1717)

[belucha]

Description

fixes #1717

MudElement is used to render a MudListItem either using a div or an a element to render the item.
This way a list and or menu item with an Href can be opened in a new browser tab using the right click menu.
MudElement had to be modified the ClickPropagation property was depending on the HtmlTag value, this logic was moved to
the MudBaseButton.

How Has This Been Tested?

• unit tests have been added
• visually

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.

[belucha]

The breaking change is in my opinion minor as it concerns MudElement with the ClickPropagation property not having internal logic.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.20%. Comparing base (28bc599) to head (827b7ae).
Report is 147 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #8649      +/-   ##
==========================================
+ Coverage   89.82%   90.20%   +0.37%     
==========================================
  Files         412      423      +11     
  Lines       11878    12274     +396     
  Branches     2364     2407      +43     
==========================================
+ Hits        10670    11072     +402     
+ Misses        681      668      -13     
- Partials      527      534       +7     

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

[belucha]

Another thing that I would like to discuss here.

What is the intended behaviour when both Href and OnClick handlers are present. I think the behaviour should be consistent across all components.

From my understanding, if Href is set the OnClick should not be called. But this decision imposes some troubles. It is for example not easily possible to add an MenuClose handler to inner MudListItem.
It even gets more complex, if we add an Target attribute (e.g. _blank) and or a ForceLoad parameter. What is the expected behaviour for a disabled button/item with an href. Should the Href then also be disabled, by forcing a render as button tag.

For my expectation it would be best to always call the OnClick handler. That might close a menu and Browser client can do a navigation (even to a new tab) -- then the close menu is desired.

I think it is quite important to have a discussion on this intended behaviour, to get a consistent design!

My Pull-Request tried to solve the problem of #1717, but it seems there are more thoughts required!

[ScarletKuro]

Sorry, I think my PR #8634 added conflicts to your PR, since I did method renaming, so was the tests affected too.

[belucha]

I've merged with dev and kept the meaning of MudElements ClickPropagation Parameter.

[belucha]

@danielchalmers I followed your recommendations and left the ClickPropagation parameter unchanged. The PreventDefault parameter was added in the same way. Even if the name imposes an inversion, I would stick with that name, as it matches the output attribute.

[danielchalmers]

If this only applies to onclick the name should probably reflect that

[belucha]

If this only applies to onclick the name should probably reflect that

Yes it only applies to onclick, but other components use the same name too. In my original pull request I named it different StopOnClickPropagation someone argued, that I should keep the name.

We could name ist OnClickPropagion but the I liked my original proposal more, because it just uses the same names as dot net uses.

Honestly I'm very uncertain here.

[henon]

What is the intended behaviour when both Href and OnClick handlers are present. I think the behaviour should be consistent
From my understanding, if Href is set the OnClick should not be called. But this decision imposes some troubles. It is for example not easily possible to add an MenuClose handler to inner MudListItem. It even gets more complex, if we add an Target attribute (e.g. _blank) and or a ForceLoad parameter. What is the expected behaviour for a disabled button/item with an href. Should the Href then also be disabled, by forcing a render as button tag.

For my expectation it would be best to always call the OnClick handler. That might close a menu and Browser client can do a navigation (even to a new tab) -- then the close menu is desired.

I think it is quite important to have a discussion on this intended behaviour, to get a consistent design!

I agree that calling the OnClick handler regardless of the Href value (except if Disabled is true) makes sense. If I attach a click handler I expect it to fire. If I want to just navigate I don't attach the handler, right?

[danielchalmers]

I agree that calling the OnClick handler regardless of the Href value (except if Disabled is true) makes sense. If I attach a click handler I expect it to fire. If I want to just navigate I don't attach the handler, right?

@henon Isn't it a problem that several other controls do it the other way?

[henon]

Yeah, that is a problem. I don't understand why it is like that. It is always a bad idea to try to "save the user from its own supposed stupidity". Most of the times you just hinder advanced use-cases for nothing.

[henon]

@belucha Please resolve conflicts. What's the status? We should get this done for v7

[belucha]

@henon I've just resolved all conflicts.
@danielchalmers I also considered your suggestions, except on regarding the rename of MudElement.ClickPropagation. I left that unchanged, since this was the original API and the the difference between OnClickPropagation and ClickPropagation is quite little.

[henon]

Just rename the private properties then it should be ready for merging.

[henon]

Thanks!

[belucha]

@henon Thanks to you and the others for the great work on this lib!

[henon]

Added to v7.0.0 Migration Guide #8447

[belucha]

@henon
I'm so sorry, I found an issue in the code just now, that was not my intention and I somehow missed it.
in MudBaseButton.cs there is a property:

        public bool ApplyClickPropagation => HtmlTag != "button" || ClickPropagation;

this should be private and matching to your convention:

        private bool GetClickPropagation() => HtmlTag != "button" || ClickPropagation;

Do you wan't me to open a new pull request, or do you change that yourself?

Sorry for this mistake.

[henon]

Don't worry, I am fixing it myself.