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

De-dupe TagHelperDescriptors based on Type for rendering. #327

Merged
merged 1 commit into from
Mar 20, 2015

Conversation

NTaylorMullen
Copy link
Member

- This can occur if you have multiple [TargetElement] attributes that overlap. Ultimately the descriptor is the same because its the same type, just the required attributes differ.
- Added tests to validate.

#326
@ghost ghost added the cla-not-required label Mar 20, 2015
@NTaylorMullen
Copy link
Member Author

Getting this in now, feel free to leave comments. I'll do a commit to address any feedback.

/cc @ajaybhargavb you should be good now 😄

@NTaylorMullen NTaylorMullen merged commit 842549b into dev Mar 20, 2015
@NTaylorMullen NTaylorMullen deleted the duplicate.targetelements.326 branch March 20, 2015 05:53
attributes: new TagHelperAttributeDescriptor[]
{
new TagHelperAttributeDescriptor("type", inputTypePropertyInfo),
new TagHelperAttributeDescriptor("checked", inputCheckedPropertyInfo)
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit: this looks like the tag helper author just wrote an extra, useless [TargetElement]. a more realistic scenario would involve something like [TargetElement(Attributes = "type")] and [TargetElement(Attributes = "checked")] on the same helper. the author didn't do anything wrong in this case but <whatever type="type" checked="checked" /> would lead to duplication.

so, minor leaning toward removing lines 45 and 64

Copy link
Member Author

Choose a reason for hiding this comment

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

Actualllly there's a flaw in the test here. If the TagHelper type is the same type the attribute descriptors will all be the same. Will update and get it into dev.

@dougbu
Copy link
Member

dougbu commented Mar 20, 2015

:shipit:

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.

2 participants