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

Blazor input component support when EditContext is not supplied #35640

Merged
merged 10 commits into from
Aug 26, 2021

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Aug 23, 2021

Blazor input component support when EditContext is not supplied

This PR enables Blazor input component functionality (InputText, InputSelect, etc.) when no EditContext is provided by a parent EditForm.

PR Description

The current approach solves the issue by allowing InputBase.EditContext to be null. Validation styling won't be applied to input components without an EditContext.

Fixes #27804

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Aug 23, 2021
@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Aug 23, 2021

Something else to consider: Do we want validation styling to apply to input components without a parent EditForm? The validation css class will always be either valid or modified valid, but this is meaningless since data annotations are ignored in these cases.

Update: One case I forgot about is if the input's current value cannot be converted to the desired type, in which case the css class would be modified invalid. So there could be an argument for keeping validation styling.

@pranavkm
Copy link
Contributor

but it has the crucial drawback that Inputbase<>.EditContext is part of the public API surface and marking it as nullable would be a breaking change

We've been playing somewhat loosey with nullable annotations as we work thru the repo. One could further argue that the current annotations are incorrect to begin with given if you create a new component, the edit context is null even though the compiler says otherwise:

ie..

var inputText = new InputText();
Console.WriteLine(inputText.EditContext.ToString()); // The compiler will claim this is null safe, even though we know this will null-ref

How does it look if we make EditContext nullable and let it be null in some cases?

@MackinnonBuck
Copy link
Member Author

We've been playing somewhat loosey with nullable annotations as we work thru the repo.

Good to know - I'll give the other approach a go.

@MackinnonBuck MackinnonBuck marked this pull request as ready for review August 24, 2021 18:21
@MackinnonBuck MackinnonBuck requested review from Pilchie and a team as code owners August 24, 2021 18:21
@MackinnonBuck
Copy link
Member Author

@pranavkm I've updated the implementation to allow EditContext to be null. Also, my previous comment...

Something else to consider: Do we want validation styling to apply to input components without a parent EditForm?

...is no longer relevant because validation styling won't happen without an edit context. It's probably better this way, since validation styling would just get in the way if no actual validation was happening.

@@ -848,6 +848,47 @@ public void InputRangeAttributeOrderDoesNotAffectValue()
Browser.Equal("210", () => rangeWithValueLast.GetDomProperty("value"));
}

[Fact]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only added tests verifying that InputSelect and InputRadioGroup work without a parent EditForm, since they contain most of the unique logic of the input components (other logic comes from InputBase). InputRadioGroup is a bit special in that it still calculates its own styling. The InputSelect test verifies that complicated binding scenarios still work (binding to arrays, boolean values).

@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Aug 24, 2021

It also appears that the CI is a bit unhappy that the baseline API files are getting modified (by making InputBase<>.EditContext nullable). We would have to make some exception for this (or revert the nullability of EditContext).

Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. @javiercn could you have a look since you said you did something similar?

src/Components/Web/src/PublicAPI.Shipped.txt Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputBase.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputBase.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputBase.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputBase.cs Show resolved Hide resolved
src/Components/Web/src/Forms/InputBase.cs Outdated Show resolved Hide resolved
@MackinnonBuck MackinnonBuck enabled auto-merge (squash) August 25, 2021 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Throw informative exception when Input Controls are not surrounded by EditForm
3 participants