Skip to content

Components topic code example updates #12893

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

Merged
merged 8 commits into from
Jun 23, 2019
Merged

Components topic code example updates #12893

merged 8 commits into from
Jun 23, 2019

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Jun 16, 2019

Fixes #12892

Skip these remarks and access the diff. My confusion was cleared in the discussion.

This PR has the final set of updates that we need.

RE: Update blazor docs to reflect directive attributes changes (aspnet/AspNetCore.Docs #12756)

  1. WRT the temporary (Pre6) need to place an @ on a field/property value of an @bind directive. I looked at Implement directive attribute feature for components (aspnet/AspNetCore #6364), but none of the description discusses the scenario.

    For example in the Format strings section of the Components topic, updates didn't add the @ to the value for StartDate ...

    <input @bind="StartDate" @bind:format="yyyy-MM-dd" />

    Same thing in the ParentYear examples (@bind-{property} syntax) ...

    <ChildComponent @bind-Year="ParentYear" />

    ... and should devs apply the @-value temporary rule to @bind-value:event/bind-{property}:event? It was left on the updates as ...

    <ChildComponent @bind-Year="ParentYear" @bind-Year:event="YearChanged" />
  2. What's the difference between the following, if any:

    <input @bind="@CurrentValue" />
    <input @bind-value="@CurrentValue" @bind-value:event="onchange" />

    Is the former just a shorthand way of setting up onchange binding? If they are the same, it isn't considered better to have Just One Consistent Way™️ 😄 to do it (the latter approach, which comports with other on{event} binding)?

@ajaybhargavb
Copy link
Contributor

@guardrex, looks like you're missing a detail about this. #12766 refers to @ not being needed for @onclick, @onfocus etc. @bind doesn't have any bug and doesn't have to start with @.

@ajaybhargavb
Copy link
Contributor

What's the difference between the following, if any:
<input @Bind="@CurrentValue" />
<input @bind-value="@CurrentValue" @bind-value:event="onchange" />
Is the former just a shorthand way of setting up onchange binding? If they are the same, it isn't considered better to have Just One Consistent Way™️ 😄 to do it (the latter approach, which comports with other on{event} binding)?

There is no difference in behavior between the two in your example. The first one is just a short version. The second format is useful if you want to specify an event other than onchange like oninput. Like in the code example that you edited. <input @bind-value="@CurrentValue" @bind-value:event="oninput" />

@guardrex
Copy link
Collaborator Author

you're missing a detail about this

Thanks ..... yes .... it all makes sense now. 😄 ...... i.e., I got 😵 on it.

@guardrex
Copy link
Collaborator Author

WRT my point number 2 .... do you think we should call that out in the topic? I could draft some text here for you to look at.

@guardrex
Copy link
Collaborator Author

@ajaybhargavb "it all makes sense" ... famous last words for my tombstone. 😄 RIP

I went back to the original PR #12756.

At https://github.com/aspnet/AspNetCore.Docs/pull/12756/files#diff-5cf16565c6014e97ab5fd792aba7b93dR24 ... it preserves the @ on _italicsCheck.

However, at https://github.com/aspnet/AspNetCore.Docs/pull/12756/files#diff-aec5790e12a05d20a3cda92a708f6679R123, it removes the @.

It looks like what we need to do here is take that @ off of the _italicsCheck in the sample app's HeadingComponent.razor.

@guardrex
Copy link
Collaborator Author

Also note that the @ is on CurrentValue for @bind-value here in that PR ... https://github.com/aspnet/AspNetCore.Docs/pull/12756/files#diff-aec5790e12a05d20a3cda92a708f6679R142 ...

<input type="text" @bind-value="@CurrentValue" @bind-value:event="oninput" />

... and my original change on this PR was to make the text match (but for @bind, which seems incorrect) ...

Using `@bind` with a `CurrentValue` property (`<input @bind="@CurrentValue" />`) is essentially 
equivalent to the following:

So, I assume that there's a difference between @bind and @bind-value in these contexts.

@ajaybhargavb
Copy link
Contributor

So, I assume that there's a difference between @Bind and @bind-value in these contexts.

Nope. Both @bind and @bind-value should be expecting a C# value (meaning doesn't have to start with @). Please correct it if I missed anything in my original PR

@guardrex
Copy link
Collaborator Author

ok ... I'll shape this PR up and ping u back.

Copy link
Contributor

@ajaybhargavb ajaybhargavb left a comment

Choose a reason for hiding this comment

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

LGTM. Please make sure the samples actually run and render properly.

@guardrex
Copy link
Collaborator Author

@ajaybhargavb kk ... that should get it. 🤞

Copy link
Member

@danroth27 danroth27 left a comment

Choose a reason for hiding this comment

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

:shipit:

@guardrex guardrex merged commit 692fb25 into master Jun 23, 2019
@delete-merged-branch delete-merged-branch bot deleted the guardrex-patch-3 branch June 23, 2019 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Components topic code example updates
3 participants