-
Notifications
You must be signed in to change notification settings - Fork 649
Remove old workaround @onclick and @bind #525
Conversation
|
||
@functions { | ||
int currentCount = 0; | ||
|
||
void IncrementCount() | ||
void IncrementCount(UIMouseEventArgs e) |
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.
I didn't mess with it, because I wanted to get some feedback first, but I had two main thoughts while doing this porting that might improve usability....
-
We could add a special case for
Action
(no arg) for the case where you don't need the args, if we think it will be common. This would be pretty easy for us to do, but we would need to push knowledge of this case through a few levels of the stack. -
We might want to consider adding
@using Microsoft.AspNetCore.Blazor
to the default usings. Now that we've got more useful stuff you'll want, we might want to do this as a practical matter. This will help keep docs and articles concise.
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.
We could add a special case for Action (no arg) for the case where you don't need the args
If it's not difficult, I think that would be really good. For click events, possibly the most common of all, you normally don't want to receive any args. It would be good to keep the tutorials simpler and cleaner-looking.
We might want to consider adding @using Microsoft.AspNetCore.Blazor to the default usings
Sounds good to me.
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.
I'm going to go ahead with this PR as is, then I'll follow up on these two items separately. The Action
one carries some risk of not making it.
/// Not used. Do not invoke this method. | ||
/// </summary> | ||
/// <returns>Always throws an exception.</returns> | ||
public virtual Task ExecuteAsync() |
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.
This was missed from my cleanup a while back
// This is the old syntax used by @bind and @onclick, it's explicitly unsupported | ||
// and has its own diagnostic. | ||
[Fact] | ||
public void OldEventHandlerSyntax_ReportsError() |
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.
Moved these tests from the rendering tests since they are now error cases.
@@ -403,7 +374,7 @@ public void SupportsTwoWayBindingForTextboxes() | |||
// Assert | |||
var frames = GetRenderTree(component); | |||
Assert.Collection(frames, | |||
frame => AssertFrame.Element(frame, "input", 3, 0), | |||
frame => AssertFrame.Element(frame, "textarea", 3, 0), |
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.
The diff is wacky. I created another test for textareas.
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.
I don't think the markup on line 368 is valid. textarea
isn't a void element. It should be <textarea bind="MyValue"></textarea>
.
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.
Cool, will fix.
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.
There were fortunately no big surprises doing this work. We were missing the bind support for textarea, but other than that, everything just lit up.
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.
Excellent!
|
||
@functions { | ||
int currentCount = 0; | ||
|
||
void IncrementCount() | ||
void IncrementCount(UIMouseEventArgs e) |
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.
We could add a special case for Action (no arg) for the case where you don't need the args
If it's not difficult, I think that would be really good. For click events, possibly the most common of all, you normally don't want to receive any args. It would be good to keep the tutorials simpler and cleaner-looking.
We might want to consider adding @using Microsoft.AspNetCore.Blazor to the default usings
Sounds good to me.
// We provide an error for this case just to be friendly. | ||
var content = string.Join("", node.Children.OfType<IntermediateToken>().Select(t => t.Content)); | ||
context.Diagnostics.Add(BlazorDiagnosticFactory.Create_CodeBlockInAttribute(node.Source, content)); | ||
return; |
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.
It's great to have this migration guidance right now, though I also think it would be good to remove it soon after 0.2.0 ships.
{ | ||
// Arrange/Act | ||
var component = CompileToComponent( | ||
@"<elem @onclick(MyHandler) /> | ||
@"<input bind=""MyValue"" /> |
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.
Previously we were talking about requiring the leading @
for bind
for consistency with events. I'm not super worried about this as we can add the requirement later if we choose, but what do you think?
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.
We can always change it later, the problem will be that folk will not get any sort of warning, it would just become HTML
@@ -403,7 +374,7 @@ public void SupportsTwoWayBindingForTextboxes() | |||
// Assert | |||
var frames = GetRenderTree(component); | |||
Assert.Collection(frames, | |||
frame => AssertFrame.Element(frame, "input", 3, 0), | |||
frame => AssertFrame.Element(frame, "textarea", 3, 0), |
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.
I don't think the markup on line 368 is valid. textarea
isn't a void element. It should be <textarea bind="MyValue"></textarea>
.
<button class="addChild" @onclick(AddChild)>Add</button> | ||
<button class="removeChild" @onclick(RemoveChild)>Remove</button> | ||
<button class="addChild" onclick="@AddChild">Add</button> | ||
<button class="removeChild" onclick="@RemoveChild">Remove</button> |
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.
Looks so much better :)
@@ -63,7 +64,7 @@ | |||
enum SelectableValue { First, Second, Third, Fourth } | |||
SelectableValue selectValue = SelectableValue.Second; | |||
|
|||
void AddAndSelectNewSelectOption() | |||
void AddAndSelectNewSelectOption(UIMouseEventArgs e) |
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.
The more of these I see, the more I think it would be great if we didn't mandate the new argument, at least for mouse events.
</p> | ||
|
||
<p> | ||
<div>Method:</div> | ||
<select id="request-method" value=@method @onchange(value => { method = (string)value; })> | ||
<select id="request-method" bind="@method"> |
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.
That is a big improvement!
</div> | ||
|
||
@functions | ||
{ | ||
@functions { |
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.
Really minor point, but I'm curious about why we often use this brace style for @functions
in Razor, but nowhere else for C# code.
Some earlier versions of Razor tooling produced pretty weird autoformatting when you pasted into a block with the opening brace on the same line, but behaved better if the opening brace was on the line below. Don't know if that's fixed yet.
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.
I think the answer is that @if (foo) {
is different than putting the {
on a separate line.
5fd4bbd
to
fe57c67
Compare
This change removes support for the old syntax used for event handlers and two-way binding. See the relevant issues for details on the new features and improvements: bind https://github.com/aspnet/Blazor/issues/409 event handlers https://github.com/aspnet/Blazor/issues/503 Along with this change we've removed a few additional things Blazor could do that aren't part of Razor's usual syntax. ---- The features that was used to make something like: ``` <button @OnClick(...) /> ``` is an expression that's embedded in a an element's attribute. This feature might be useful in the future if we want to support 'splatting' arbitrary attributes into a tag, but the runtime support for this isn't accessible outside the Blazor core. ---- The features that implement: ``` <button onclick=@{ } /> ``` have been removed in favor of a better design for lambdas, method group conversions and other things for event handler attributes. use `<button onclick=@(x => ...} />` instead. We think is a better approach in general, because we want the app developer to write and see the parameter list. ---- Both syntactic features that have been removed have dedicated error messages in the compiler. If you're porting old code it should help you figure out what to do.
fe57c67
to
cda57e7
Compare
This change removes support for the old syntax used for event handlers
and two-way binding.
See the relevant issues for details on the new features and
improvements:
bind https://github.com/aspnet/Blazor/issues/409
event handlers https://github.com/aspnet/Blazor/issues/503
Along with this change we've removed a few additional things Blazor
could do that aren't part of Razor's usual syntax.
The features that was used to make something like:
is an expression that's embedded in a an element's attribute. This
feature might be useful in the future if we want to support 'splatting'
arbitrary attributes into a tag, but the runtime support for this isn't
accessible outside the Blazor core.
The features that implement:
have been removed in favor of a better design for lambdas, method group
conversions and other things for event handler attributes.
use
<button onclick=@(x => ...} />
instead.We think is a better approach in general, because we want the app
developer to write and see the parameter list.
Both syntactic features that have been removed have dedicated error
messages in the compiler. If you're porting old code it should help you
figure out what to do.