Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MudDropZone: Add nullable annotation and fix #6551, #4695. #6561

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Mar 30, 2023

Description

Fixes #6551 and fixes #4695, and fixes #4489
Also contributes to this #6535

Important notes about PR

1.I have moved:

  • MudDragAndDropIndexChangedEventArgs
  • MudDragAndDropItemTransaction
  • MudDragAndDropTransactionFinishedEventArgs
  • MudItemDropInfo

To their own files, reasoning: the MudDropContainer.razor.cs was somehow way too big, it was hard to navigate and go through the code. I think it's justified considering that there are no open pull requests on this component(there is one, but it already has conflicts and needs rework).

2.There were some typo fixes, but only the comments and methods that are private. The public one will be a separate PR.

3.private IEnumerable<T> GetItems() was changed to T[] GetItems().
It was already calling .ToArray() but then for some reason it was casted back to IEnumerable. The problem with this is that in places that called this method it was doing multiple enumeration which is a bad practice, it's better to keep it array since it was already converted to array by the original author of the code.

4.Coverage might drop, since there is constant check if _transaction and container is null. But those are important, especially considering that multiple methods can manipulate the _transcation value at the same time, which is why there was a problem.

How Has This Been Tested?

Current unit tests and manual testing on the docs, tested the on this scenario too https://try.mudblazor.com/snippet/mkGHOxGtQttdZNao.

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.

@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Mar 30, 2023
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage: 79.78% and project coverage change: -0.08 ⚠️

Comparison is base (465f37b) 91.46% compared to head (7add5ad) 91.39%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #6561      +/-   ##
==========================================
- Coverage   91.46%   91.39%   -0.08%     
==========================================
  Files         393      397       +4     
  Lines       14876    14915      +39     
==========================================
+ Hits        13607    13631      +24     
- Misses       1269     1284      +15     
Impacted Files Coverage Δ
...or/Components/DropZone/MudDynamicDropItem.razor.cs 34.83% <20.00%> (-4.13%) ⬇️
...azor/Components/DropZone/MudDropContainer.razor.cs 89.18% <87.75%> (-7.29%) ⬇️
...ts/DropZone/MudDragAndDropIndexChangedEventArgs.cs 100.00% <100.00%> (ø)
...mponents/DropZone/MudDragAndDropItemTransaction.cs 100.00% <100.00%> (ø)
...Zone/MudDragAndDropTransactionFinishedEventArgs.cs 100.00% <100.00%> (ø)
...rc/MudBlazor/Components/DropZone/MudDropZone.razor 100.00% <100.00%> (ø)
...MudBlazor/Components/DropZone/MudDropZone.razor.cs 100.00% <100.00%> (+0.77%) ⬆️
...lazor/Components/DropZone/MudDynamicDropItem.razor 100.00% <100.00%> (ø)
...c/MudBlazor/Components/DropZone/MudItemDropInfo.cs 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@ScarletKuro
Copy link
Member Author

@henon

@ScarletKuro
Copy link
Member Author

I was browsing the issue list of MudBlazor and discovered this issue #4695, this PR covers this as well.

@ScarletKuro ScarletKuro changed the title MudDropZone: Add nullable annotation and fix #6551. MudDropZone: Add nullable annotation and fix #6551, #4695. Mar 30, 2023
@ScarletKuro
Copy link
Member Author

ScarletKuro commented Mar 30, 2023

Idea for v7.
In my opinion, when Container is null it should throw an exception, rather than checking for null and skip doing the job.
Because the MudDropZone can't live outside the MudDropContainer. It requires the container to function.
This is why when container is null

[CascadingParameter]
protected MudDropContainer<T>? Container { get; set; }

Means it's an abnormal situation.
This would significantly improve the code and as well we would be able to throw a meaningful error instead of just checking for null and have no action. I like the "fail fast" when something is wrong more.

But this would mean a little breaking change, so I'd move to v7.

I would also maybe consider adding a "class" constraint, this would allow some better null checks, I don't think u can use MudDropContainer<string>, tho I didn't check yet.

@henon
Copy link
Collaborator

henon commented Mar 31, 2023

when Container is null it should throw an exception, rather than checking for null and skip doing the job.
Because the MudDropZone can't live outside the MudDropContainer. It requires the container to function.

Done, added to v7 project: https://github.com/MudBlazor/MudBlazor/projects/13#card-88710515

@henon henon merged commit eaf5d78 into MudBlazor:dev Mar 31, 2023
@henon henon added enhancement New feature or request and removed PR: needs review labels Mar 31, 2023
@henon
Copy link
Collaborator

henon commented Mar 31, 2023

Good job Kuro, thanks!

@ScarletKuro ScarletKuro deleted the dropzone_nullable_and_fix6551 branch April 1, 2023 20:00
ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected enhancement New feature or request
Projects
None yet
2 participants