Skip to content

Expose ReferenceElement for qualified input components#24899

Closed
uabarahona wants to merge 9 commits into
dotnet:mainfrom
uabarahona:Add_InputElement_to_qualified_classes
Closed

Expose ReferenceElement for qualified input components#24899
uabarahona wants to merge 9 commits into
dotnet:mainfrom
uabarahona:Add_InputElement_to_qualified_classes

Conversation

@uabarahona
Copy link
Copy Markdown
Contributor

@uabarahona uabarahona commented Aug 14, 2020

Summary

As discussed on #24446 this PR is to expose "InputElement" which is a ReferenceElement, some notes to take into account:

  • @MackinnonBuck I have stick with InputElement name everywhere to keep consistency. [I think it's fine as select is also a input, but I can change the name for select if it is really needed]
  • I have made the set protected to support inheritance

Addresses #24446

@uabarahona uabarahona requested review from a team and SteveSandersonMS as code owners August 14, 2020 01:40
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Aug 14, 2020
@uabarahona uabarahona changed the title Add inputelement to qualified classes Expose ReferenceElement for qualified input components Aug 14, 2020
Comment thread src/Components/Web/src/Forms/InputCheckbox.cs Outdated
Copy link
Copy Markdown
Member

@MackinnonBuck MackinnonBuck left a comment

Choose a reason for hiding this comment

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

Looks good! I had a couple small comments. @pranavkm, could you take a look?

Comment thread src/Components/Web/src/Forms/InputCheckbox.cs Outdated
Comment thread src/Components/Web/src/Forms/InputSelect.cs Outdated
Comment thread src/Components/Web/src/Forms/InputCheckbox.cs Outdated
@captainsafia captainsafia added the community-contribution Indicates that the PR has been added by a community member label Aug 14, 2020
Copy link
Copy Markdown
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.

This looks really good. Any chance we could add one test that verifies this works end-to-end? Something that verifies that you could Focus on the input element using this mechanism?

@uabarahona uabarahona force-pushed the Add_InputElement_to_qualified_classes branch from b5d30a2 to 6db9c7d Compare August 14, 2020 23:14
@uabarahona
Copy link
Copy Markdown
Contributor Author

uabarahona commented Aug 14, 2020

Tests were added and they brought up an issue where InputRadio was being re-rendered always no matter what, this due InputRadioGroup sets InputRadioContext as key but InputRadioContext didn't have a way to compare so the diff process was taking the InputRadioGroups always as different.

private InputRadioContext? _context;

builder.OpenComponent<CascadingValue<InputRadioContext>>(0);
builder.SetKey(_context);
builder.AddAttribute(1, "IsFixed", true);

@uabarahona uabarahona requested a review from pranavkm August 14, 2020 23:20
@MackinnonBuck
Copy link
Copy Markdown
Member

@barahonajm The problem you're describing is intended behavior - the radio buttons are re-rendered when the _context changes so validation styling is applied. We might be able to investigate a way to preserve the radio button instances when re-rendering so ElementReferences don't get invalidated. Was the issue in using the ElementReference itself or just the E2E tests (i.e. StaleElementReferenceException)?

@uabarahona
Copy link
Copy Markdown
Contributor Author

uabarahona commented Aug 14, 2020

@MackinnonBuck I see, let me ask a little question shouldn't the radiogroup be re-renderd only when the _context changes? because in my case both _context old and new have exactly the same content but since they are always initialized as a new class they appear as different, that is because I added a equals to InputRadioContext.

Here the code on InputRadioGroup:

protected override void OnParametersSet()
{
    var groupName = !string.IsNullOrEmpty(Name) ? Name : _defaultGroupName;
    var fieldClass = EditContext.FieldCssClass(FieldIdentifier);
    var changeEventCallback = EventCallback.Factory.CreateBinder<string?>(this, __value => CurrentValueAsString = __value, CurrentValueAsString);

    _context = new InputRadioContext(CascadedContext, groupName, CurrentValue, fieldClass, changeEventCallback);
}

The issue on E2E test is that the FocusAsync() does not work at all for radio buttons since they get re-render after the focus is applied

@MackinnonBuck
Copy link
Copy Markdown
Member

@barahonajm it must be because a new InputRadioContext is created each time the parameters are set, so it is therefore a new key. One solution could be to get rid of the IsFixed attribute for the CascadingValue and remove the key as well, but I'm not completely sure what the performance implications of this are. @pranavkm, do you have any suggestions for this?

@uabarahona
Copy link
Copy Markdown
Contributor Author

uabarahona commented Aug 14, 2020

Yes, that's the reason I was not able to explain it well 😃, I added an equals to InputRadioContext and that solved the issue, but I don't know too the performance implications

@uabarahona uabarahona force-pushed the Add_InputElement_to_qualified_classes branch from 6db9c7d to 7f7873b Compare August 19, 2020 03:33
@pranavkm pranavkm added this to the 6.0.0-alpha1 milestone Aug 19, 2020
@pranavkm
Copy link
Copy Markdown
Contributor

We discussed options and think it might be easier to not have the InputRadio.ElementReference to start with. We can always fix the implementation in the future to have this work.

@uabarahona
Copy link
Copy Markdown
Contributor Author

Got it, I have removed the Inputradio from the PR

@uabarahona uabarahona force-pushed the Add_InputElement_to_qualified_classes branch from 5a083aa to 6392ae3 Compare August 20, 2020 01:08
@pranavkm pranavkm self-assigned this Aug 26, 2020
@SteveSandersonMS
Copy link
Copy Markdown
Member

@pranavkm When completing this PR, please also consider adding support for #28533 and closing that as resolved.

Base automatically changed from master to main January 22, 2021 01:32
@pranavkm
Copy link
Copy Markdown
Contributor

Thanks @JuanElements for your PR. I've created another PR based on your changes here - #29555. We'll use that to track this to completion.

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 community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants