Add OnClick parameter to BreadcrumbItem, Add Tests, Remove unusable code #2655
Conversation
… BreadcrumbItem. Remove unused and unreachable code until needed at a later date. Documented parameters not in use, but left to avoid breaking the existing public API.
|
|
Codecov Report
@@ Coverage Diff @@
## master #2655 +/- ##
==========================================
+ Coverage 30.45% 32.24% +1.78%
==========================================
Files 505 543 +38
Lines 34387 26451 -7936
Branches 0 260 +260
==========================================
- Hits 10473 8529 -1944
+ Misses 23914 17882 -6032
- Partials 0 40 +40
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| [CascadingParameter] | ||
| public Breadcrumb BreadCrumb { get; set; } | ||
| [Parameter] | ||
| public EventCallback<MouseEventArgs> OnClick { get; set; } |
There was a problem hiding this comment.
I think it's better to pass in the current BreadcrumbOption.
There was a problem hiding this comment.
Should we pass back the BreadcrumbItem itself? From what I could tell, the BreadcrumbOption class is not used anywhere yet. I just didn't remove it because it was public.
There was a problem hiding this comment.
Oh, that fine, but it's better to pass some info of current item to user
There was a problem hiding this comment.
Yeah, makes sense. I went ahead and did a Tuple<MouseEventArgs, BreadcrumbItem> so the user has access to the click arguments and the item since both have useful information. How does that look?
There was a problem hiding this comment.
That's fine. Do you plan to fix HTML differences(#2658) here?
There was a problem hiding this comment.
I was thinking of doing that in a separate PR after these changes get in. Would that just be PR'd to master or somewhere else since it will completely change the HTML for existing users? Wasn't sure if that would be considered a breaking change or not.
There was a problem hiding this comment.
I think this is a breaking change for CSS, not for HTML or Blazor. CSS has been updated in the latest version, but HTML hasn't caught up.
There was a problem hiding this comment.
It was a pretty small change to update the HTML markup so I went ahead and did that too. It is now in this PR.
…ventArgs, BreadcrumbItem> giving access to the click arguments and the item clicked.
…, ol and li elements instead of div and spans only)
|
|
||
| private async void _onClick(MouseEventArgs args) |
There was a problem hiding this comment.
It's better to name HandleOnClick
There was a problem hiding this comment.
Ok. I see you changed the signature of the OnClick to just be MouseEventArgs now though. This method isn't needed anymore then since you can directly pass OnClick to the element.
…d Tests, Remove unusable code (#2655) * Add OnClick parameter to BreadcrumbItem. Add tests for Breadcrumb and BreadcrumbItem. Remove unused and unreachable code until needed at a later date. Documented parameters not in use, but left to avoid breaking the existing public API. * Fix whitespace * Update signature of OnClick for BreadcrumbItem to pass a Tuple<MouseEventArgs, BreadcrumbItem> giving access to the click arguments and the item clicked. * Add ExludeFromCodeCoverage to POCO * Update HTML markup for Breadcrumbs to match React Ant.Design (use nav, ol and li elements instead of div and spans only) * fix naming * fix error * fix test Co-authored-by: James Yeung <shunjiey@hotmail.com>
🤔 This is a ...
🔗 Related issue link
#2540 - OnClick
#2649 - Remove unusable code
#2644 - Test coverage
#2658 - Wrong HTML markup causing last separator to display
💡 Background and solution
📝 Changelog
☑️ Self Check before Merge