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

Commit

Permalink
Addressed code review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
NTaylorMullen committed Oct 22, 2015
1 parent 2089f0c commit 96421a5
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ public class RenderAtEndOfFormTagHelper : TagHelper
/// <inheritdoc />
public override void Init(TagHelperContext context)
{
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}

// Push the new FormContext.
ViewContext.FormContext = new FormContext
{
Expand Down
59 changes: 34 additions & 25 deletions src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public class SelectTagHelper : TagHelper
{
private const string ForAttributeName = "asp-for";
private const string ItemsAttributeName = "asp-items";
private bool _allowMultiple;
private IReadOnlyCollection<string> _currentValues;

/// <summary>
/// Creates a new <see cref="SelectTagHelper"/>.
Expand Down Expand Up @@ -59,27 +61,16 @@ public override int Order
[HtmlAttributeName(ItemsAttributeName)]
public IEnumerable<SelectListItem> Items { get; set; }

/// <inheritdoc />
/// <remarks>Does nothing if <see cref="For"/> is <c>null</c>.</remarks>
/// <exception cref="InvalidOperationException">
/// Thrown if <see cref="Items"/> is non-<c>null</c> but <see cref="For"/> is <c>null</c>.
/// </exception>
public override void Process(TagHelperContext context, TagHelperOutput output)
public override void Init(TagHelperContext context)
{
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}

if (output == null)
{
throw new ArgumentNullException(nameof(output));
}

// Note null or empty For.Name is allowed because TemplateInfo.HtmlFieldPrefix may be sufficient.
// IHtmlGenerator will enforce name requirements.
var metadata = For.Metadata;
if (metadata == null)
if (For.Metadata == null)
{
throw new InvalidOperationException(Resources.FormatTagHelpers_NoProvidedMetadata(
"<select>",
Expand All @@ -92,36 +83,54 @@ public override void Process(TagHelperContext context, TagHelperOutput output)
// "SelectExpressionNotEnumerable" InvalidOperationException during generation.
// Metadata.IsEnumerableType is similar but does not take runtime type into account.
var realModelType = For.ModelExplorer.ModelType;
var allowMultiple = typeof(string) != realModelType &&
_allowMultiple = typeof(string) != realModelType &&
typeof(IEnumerable).GetTypeInfo().IsAssignableFrom(realModelType.GetTypeInfo());
_currentValues = Generator.GetCurrentValues(
ViewContext,
For.ModelExplorer,
expression: For.Name,
allowMultiple: _allowMultiple);

// Whether or not (not being highly unlikely) we generate anything, could update contained <option/>
// elements. Provide selected values for <option/> tag helpers. They'll run next.
context.Items[typeof(SelectTagHelper)] = _currentValues;
}

/// <inheritdoc />
/// <remarks>Does nothing if <see cref="For"/> is <c>null</c>.</remarks>
/// <exception cref="InvalidOperationException">
/// Thrown if <see cref="Items"/> is non-<c>null</c> but <see cref="For"/> is <c>null</c>.
/// </exception>
public override void Process(TagHelperContext context, TagHelperOutput output)
{
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}

if (output == null)
{
throw new ArgumentNullException(nameof(output));
}

// Ensure GenerateSelect() _never_ looks anything up in ViewData.
var items = Items ?? Enumerable.Empty<SelectListItem>();

var currentValues = Generator.GetCurrentValues(
ViewContext,
For.ModelExplorer,
expression: For.Name,
allowMultiple: allowMultiple);
var tagBuilder = Generator.GenerateSelect(
ViewContext,
For.ModelExplorer,
optionLabel: null,
expression: For.Name,
selectList: items,
currentValues: currentValues,
allowMultiple: allowMultiple,
currentValues: _currentValues,
allowMultiple: _allowMultiple,
htmlAttributes: null);

if (tagBuilder != null)
{
output.MergeAttributes(tagBuilder);
output.PostContent.Append(tagBuilder.InnerHtml);
}

// Whether or not (not being highly unlikely) we generate anything, could update contained <option/>
// elements. Provide selected values for <option/> tag helpers. They'll run next.
context.Items[typeof(SelectTagHelper)] = currentValues;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@
<option value="HtmlEncode[[Credit]]">Credit</option>
<option value="HtmlEncode[[Check]]" selected="HtmlEncode[[selected]]">Check</option>
</select>
<select>
<option value="HtmlEncode[[Check]]">Check</option>
</select>
</div>
<div>
<label class="order" for="HtmlEncode[[Customer_Number]]">HtmlEncode[[Number]]</label>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
<select id="HtmlEncode[[PaymentMethod]]" multiple="HtmlEncode[[multiple]]" name="HtmlEncode[[PaymentMethod]]"><option value="HtmlEncode[[Credit]]">HtmlEncode[[Credit]]</option>
<option selected="HtmlEncode[[selected]]" value="HtmlEncode[[Check]]">HtmlEncode[[Check]]</option>
</select>
<select>
<option value="Check">Check</option>
</select>
</div>
<div>
<label class="HtmlEncode[[order]]" for="HtmlEncode[[Customer_Number]]">HtmlEncode[[Number]]</label>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
<select id="PaymentMethod" multiple="multiple" name="PaymentMethod"><option value="Credit">Credit</option>
<option selected="selected" value="Check">Check</option>
</select>
<select>
<option value="Check">Check</option>
</select>
</div>
<div>
<label class="order" for="Customer_Number">Number</label>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ public async Task ProcessAsync_GeneratesExpectedOutput(
};

// Act
tagHelper.Init(tagHelperContext);
await tagHelper.ProcessAsync(tagHelperContext, output);

// Assert
Expand Down Expand Up @@ -332,6 +333,7 @@ public async Task ProcessAsync_WithItems_GeneratesExpectedOutput_DoesNotChangeSe
};

// Act
tagHelper.Init(tagHelperContext);
await tagHelper.ProcessAsync(tagHelperContext, output);

// Assert
Expand Down Expand Up @@ -427,7 +429,6 @@ public async Task ProcessAsyncInTemplate_WithItems_GeneratesExpectedOutput_DoesN
var savedSelected = items.Select(item => item.Selected).ToList();
var savedText = items.Select(item => item.Text).ToList();
var savedValue = items.Select(item => item.Value).ToList();

var tagHelper = new SelectTagHelper(htmlGenerator)
{
For = modelExpression,
Expand All @@ -436,6 +437,7 @@ public async Task ProcessAsyncInTemplate_WithItems_GeneratesExpectedOutput_DoesN
};

// Act
tagHelper.Init(tagHelperContext);
await tagHelper.ProcessAsync(tagHelperContext, output);

// Assert
Expand Down Expand Up @@ -533,6 +535,7 @@ public async Task ProcessAsync_CallsGeneratorWithExpectedValues_ItemsAndAttribut
};

// Act
tagHelper.Init(tagHelperContext);
await tagHelper.ProcessAsync(tagHelperContext, output);

// Assert
Expand Down Expand Up @@ -606,6 +609,7 @@ public async Task TagHelper_CallsGeneratorWithExpectedValues_RealModelType(
};

// Act
tagHelper.Init(tagHelperContext);
await tagHelper.ProcessAsync(tagHelperContext, output);

// Assert
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ Html.BeginForm(actionName: "Submit", controllerName: "HtmlGeneration_Order"))
<div>
@Html.LabelFor(m => m.PaymentMethod, htmlAttributes: new { @class = "order" })
@Html.ListBoxFor(m => m.PaymentMethod, selectList: new SelectList(new[] { new { value = "Credit" }, new { value = "Check" } }, dataValueField: "value", dataTextField: "value"))
<select>
<option value="Check">Check</option>
</select>
</div>
<div>
@Html.LabelFor(m => m.Customer.Number, htmlAttributes: new { @class = "order" })
Expand Down

0 comments on commit 96421a5

Please sign in to comment.