-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
MudDialogProvider: Add missing BackgroundClass (#8454) #8458
Conversation
public class DialogOptions
{
public DialogPosition? Position { get; set; }
public MaxWidth? MaxWidth { get; set; }
public bool? DisableBackdropClick { get; set; }
public bool? CloseOnEscapeKey { get; set; }
public bool? NoHeader { get; set; }
public bool? CloseButton { get; set; }
public bool? FullScreen { get; set; }
public bool? FullWidth { get; set; }
public string? ClassBackground { get; set; }
} I don't understand, you don't accept the possibility of a null on a string? The parameter may not be present, so it should be null, it is optional. |
That string should be nullable by default because it's not in a '#nullable' annotations context. Try dropping the |
@danielchalmers I have enabled nullable for these two files and fixed the errors with null missing. We should support null, even Microsoft recommends that we have nullable enabled for the entire project. |
Please see #6535 if you're interested in helping in this area |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #8458 +/- ##
=======================================
Coverage 88.84% 88.85%
=======================================
Files 416 416
Lines 12358 12359 +1
Branches 2458 2458
=======================================
+ Hits 10980 10981 +1
Misses 845 845
Partials 533 533 ☔ View full report in Codecov by Sentry. |
In a few days I will have a little free time so I can help a little. |
I know that'll be very appreciated! |
Please rename it to @ArieGato will change the property name in dialog provider in his breaking-change PR to |
@henon |
I know we do this all the time. But the next release will be v7.0.0 where we finally after waiting for years remove all obsolete properties and make breaking changes. This will be only one in a dozen breaking changes and there will be a migration guide: #8447 |
In any case, you can rename the property you added without a breaking change, so please do so. |
@henon Ok, I understand. Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ScarletKuro You may merge after taking a second look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I guess this is breaking change |
Added to the v7.0.0 Migration Guide |
Description
resolves #8454
How Has This Been Tested?
Types of changes
Checklist:
dev
).