Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

Add buttonTagHelper and submitTagHelper for formaction #5089

Closed
wants to merge 1 commit into from
Closed

Add buttonTagHelper and submitTagHelper for formaction #5089

wants to merge 1 commit into from

Conversation

ctyar
Copy link
Contributor

@ctyar ctyar commented Jul 28, 2016

Addresses #1668

@@ -147,4 +147,16 @@
<data name="PropertyOfTypeCannotBeNull" xml:space="preserve">
<value>The '{0}' property of '{1}' must not be null.</value>
</data>
<data name="ButtonTagHelper_CannotOverrideFormAction" xml:space="preserve">
Copy link
Member

Choose a reason for hiding this comment

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

Suggest renaming existing resources e.g. AnchorTagHelper_CannotOverrideHref and reusing them here. Resources are internal so there's no compat worry. And maintenance will be easier if we keep the numbers and repetitions down.

Copy link
Member

Choose a reason for hiding this comment

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

🙈 Never mind. "a" versus "an" and the different ordering of the FormTagHelper_CannotOverrideAction parameters makes this difficult.

@dougbu
Copy link
Member

dougbu commented Jul 30, 2016

I haven't looked at the tests in detail. But the general direction is very solid.

One question: The <a> and <form> tag helpers already duplicate each other a fair amount. The new <button> and <submit> helpers both look almost identical to the <a> one. Could we refactor things e.g. to a static class? That way, future fixes will be done everywhere and we won't miss a tag helper or three? (Can't change the existing tag helpers to have a base class because that would be a breaking change and @rynowak would hate me forever more.)

@Eilon
Copy link
Member

Eilon commented Aug 17, 2016

@dougbu assigning this to you since you already started looking at it.

@ctyar - sorry for the delay getting back to you. Do you have thoughts on @dougbu 's comments?

@dougbu dougbu mentioned this pull request Aug 30, 2016
@dougbu
Copy link
Member

dougbu commented Aug 30, 2016

@ctyar thanks very much for your submission. Please see #5201 for the direction we're taking this work. Your commit remains and will be part of the final push to GitHub of course.

@dougbu dougbu closed this Aug 30, 2016
@ctyar
Copy link
Contributor Author

ctyar commented Aug 30, 2016

@dougbu Sorry that couldn't finish the work, I hope this does not cause any inconvenience to you guys

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants