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

Add ValidationSummaryTagHelper. #1340

Conversation

NTaylorMullen
Copy link
Member

[ContentBehavior(ContentBehavior.Append)]
public class ValidationSummaryTagHelper : TagHelper
{
private static readonly bool DefaultModelErrorsOnlyBehavior = false;
Copy link
Member

Choose a reason for hiding this comment

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

rename w/o "Behavior"

@dougbu
Copy link
Member

dougbu commented Oct 13, 2014

@NTaylorMullen NTaylorMullen changed the title [Design] Add ValidationSummaryTagHelper. Add ValidationSummaryTagHelper. Oct 14, 2014
@NTaylorMullen NTaylorMullen force-pushed the TagHelpers_ValidationSummary branch 2 times, most recently from c490007 to c8615e9 Compare October 14, 2014 05:37
@NTaylorMullen
Copy link
Member Author

Addressed code review comments to the exception of the one comment with differing opinions 😄

Assert.Equal("div", output.TagName);
Assert.Empty(output.Attributes);

if (!validationSummary.Equals("None", StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

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

split into two tests rather than conditionalizing the Asserts

@dougbu
Copy link
Member

dougbu commented Oct 16, 2014

@NTaylorMullen
Copy link
Member Author

Addressed code review comments.

@NTaylorMullen
Copy link
Member Author

Update TagHelper sample to use new validation-summary format.


// TODO: Change to ValidationSummary enum once https://github.com/aspnet/Razor/issues/196 has been completed.
/// <summary>
/// Acceptable values are defined by the <see cref="ValidationSummary"/> enum.
Copy link
Member

Choose a reason for hiding this comment

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

good second sentence in this <summary/>. restore first sentence from an earlier iteration to describe that this property is for.

@dougbu
Copy link
Member

dougbu commented Oct 16, 2014

⌚ just for a quick look at doc comments in next iteration

Resources.FormatValidationSummaryTagHelper_InvalidValidationSummaryValue(
"validation-summary",
ValidationSummaryValue,
ValidationSummary.All.ToString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't have to do ToString(), the string.Format would do that for you.

@pranavkm
Copy link
Contributor

:shipit: once @dougbu's happy

@NTaylorMullen
Copy link
Member Author

Updated.

@@ -126,4 +126,7 @@
<data name="FormTagHelper_CannotDetermineAction" xml:space="preserve">
<value>Cannot determine an {1} for {0}. A {0} with a URL-based {1} must not have attributes starting with {3} or a {2} attribute.</value>
</data>
<data name="ValidationSummaryTagHelper_InvalidValidationSummaryValue" xml:space="preserve">
<value>Cannot parse {0} value '{1}'. Acceptable values are '{2}', '{3}' and '{4}'.</value>
Copy link
Member

Choose a reason for hiding this comment

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

nit: passing your usual rule back 😸 add "for {2}" just before the first period

Copy link
Member

Choose a reason for hiding this comment

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

quote current {0} as well -> '{0}' (though that should probably become {1} and so on

@dougbu
Copy link
Member

dougbu commented Oct 17, 2014

:shipit: after addressing my 2 (or so) minor comments

- Tested ValidationSummaryTagHelper behavior.
- Updated sample to utilize new ValidationSummaryTagHelper format.

#1251
@NTaylorMullen
Copy link
Member Author

Done in TagHelpersFeature via ba111a4

@NTaylorMullen NTaylorMullen deleted the TagHelpers_ValidationSummary branch October 17, 2014 05:04
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.

3 participants