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

Should we use Action<T> or EventCallback<T> for binding? #12126

Closed
Andrzej-W opened this issue Apr 23, 2019 — with docs.microsoft.com · 8 comments · Fixed by #12887
Closed

Should we use Action<T> or EventCallback<T> for binding? #12126

Andrzej-W opened this issue Apr 23, 2019 — with docs.microsoft.com · 8 comments · Fixed by #12887
Assignees
Labels
Blazor Pri2 Priority 2 Source - Docs.ms Docs Customer feedback via GitHub Issue
Milestone

Comments

Copy link

Andrzej-W commented Apr 23, 2019

In child component example in "Data binding" section you have this code:

@functions {
    [Parameter]
    private int Year { get; set; }
    [Parameter]
    private Action<int> YearChanged { get; set; }
}

Near the end of "Event handling" section we can read:

Use EventCallback and EventCallback<T> for event handling and binding component parameters.

So the question is: which type should we use for binding?


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@dotnet-bot dotnet-bot added the Source - Docs.ms Docs Customer feedback via GitHub Issue label Apr 23, 2019
@guardrex
Copy link
Collaborator

guardrex commented Apr 23, 2019

[EDIT] Cleaning this up. Thought the question was "which type should we use for binding?" WRT EventCallback vs. EventCallback<T>. The PR will address removal of Action<T>.

However WRT what we have on EventCallback and EventCallback<T> ...

Prefer the strongly typed EventCallback<T>, which provides better error feedback to users of the component. Similar to other UI event handlers, specifying the event parameter is optional. Use EventCallback when there's no value passed to the callback.

The last sentence makes sense.

The part that I find a hair cryptic is the "better error feedback" bit. It leaves me wondering what kind of improved feedback is provided and for what types of errors? I imagine that we can be more specific there.

I'll mark this for engineering to take a look and respond. No ETA on a response, but we should get an answer soon-ish.

@guardrex guardrex self-assigned this Apr 23, 2019
@guardrex guardrex added the DR label Apr 23, 2019
@guardrex guardrex added this to the Backlog milestone Apr 23, 2019
@guardrex
Copy link
Collaborator

guardrex commented Apr 23, 2019

... oh ... good 👁️ on Action<T> ... let me work that one now on the current open PR.

@Andrzej-W
Copy link
Author

@guardrex it looks that you are confused with my question

which type should we use for binding?

which is written immediately below this text

Use EventCallback and EventCallback<T> for event handling and binding component parameters.

I'm sorry, it is really confusing. The real and most important question is in the title of this issue:

Should we use Action<T> or EventCallback<T> for binding?

and I see that you have already made a correction.
BTW EventCallback is really well described in this issue: dotnet/aspnetcore#6351

@guardrex
Copy link
Collaborator

guardrex commented Apr 24, 2019

That's right ... At first, I was 🏃 too fast and only looked at the last two lines. The Action<T> part is resolved on the PR.

dotnet/aspnetcore#6351 was used to generate the text, but a lot of that language doesn't lend itself to use in a reference document of this nature. Most devs just want to know what is it?, when do I use it?, how do I use it?, and what are the gotchas?. The discussion on the issue gets into a lot of history and implementation details that we probably don't want in the doc. However, I still find our current doc text a hair cryptic. Engineering will let us know if/what changes to make.

Let's leave this open. I marked it for triage.

@guardrex guardrex added the Blazor label May 5, 2019
@guardrex guardrex added Pri2 Priority 2 and removed DR labels May 30, 2019
@guardrex
Copy link
Collaborator

guardrex commented Jun 6, 2019

@Andrzej-W It seems the PR that I put in almost completely resolved this. Engineering (and docs) are 100% 👍 with EventCallback coverage now.

  • Action<T> is outta there! ⚾️ That's updated to EventCallback.
  • For EventCallback versus EventCallback<T>, we have:
    • EventCallback<T> is strongly typed and requires a specific argument type. EventCallback is weakly typed and allows any argument type.

    • Use EventCallback when there's no value passed to the callback.

We still have that generic remark that "EventCallback<T> ... provides better error feedback to users of the component." However, that's a small, low priority thing to address. I'm 🏃 at the moment. 😅

Is there anything else significant that requires attention?

@Andrzej-W
Copy link
Author

@guardrex In Event handling section, EventCallback subsection below second example we have this sentence:

When the button is selected in the Child component:

I think that selected should be changed to clicked. Selected in case of button for me means button got focus but here button have to be clicked (or tapped on touchscreen).

A few lines below we have this sentence:

Don't use EventCallback and EventCallback for child content—continue to use RenderFragment and RenderFragment for child content.

I have to say that I don't understand this sentence at all. It suggests that EventCallback has something in common with RenderFragment and people can confuse them. Maybe someone was thinking about similarities between EventCallback and Action but of course we don't use them for child content. Maybe we should ask someone for a good example when to use Action instead of EventCallback and then rephrase this sentence or remove it completely.

@guardrex
Copy link
Collaborator

guardrex commented Jun 7, 2019

I think that selected should be changed to clicked. Selected in case of button for me means button got focus but here button have to be clicked (or tapped on touchscreen).

Can't change that ... Microsoft documentation guidelines call for "selected." We have to say "focused" (for focused) and "selected" (for clicked).

Don't use EventCallback and EventCallback for child content—continue to use RenderFragment and RenderFragment for child content.

@danroth27, do we need this retain this remark?

@danroth27
Copy link
Member

Don't use EventCallback and EventCallback for child content—continue to use RenderFragment and RenderFragment for child content.

@danroth27, do we need this retain this remark

I think this sentence can be removed, especially if it's causing more confusion than good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blazor Pri2 Priority 2 Source - Docs.ms Docs Customer feedback via GitHub Issue
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants