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

Consume EditorBrowsableAttribute on TagHelpers. #448

Merged
merged 1 commit into from
Jul 16, 2015

Conversation

NTaylorMullen
Copy link
Contributor

  • Changed TagHelperDescriptorFactory to not create individual descriptors when EditorBrowsableAttribute is present and set to EditorBrowsableState.Never.
  • Added tests to validate the TagHelperDescriptorFactory creates the attribute correctly.
  • Did not look down the inheritance chain for EditorBrowsableAttribute because TargetElement is not inherited.
    Consume EditorBrowsableAttribute on TagHelpers. #447

@NTaylorMullen
Copy link
Contributor Author

/cc @dougbu @Eilon

@@ -38,13 +39,20 @@ public static TagHelperDesignTimeDescriptor CreateDescriptor([NotNull] Type type
?.OfType<OutputElementHintAttribute>()
.FirstOrDefault();
var outputElementHint = outputElementHintAttribute?.OutputElement;
var editorBrowsableAttribute = type
.GetCustomAttributes(inherit: false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not looking down the inheritance chain here because TargetElement isn't inherited. However, the default inheritance behavior of EditorBrowsableAttribute is true. I'm open to opinions 😄

@NTaylorMullen NTaylorMullen force-pushed the nimullen/editorbrowsable.447 branch 2 times, most recently from 140bb31 to 50d798b Compare July 13, 2015 23:22
@NTaylorMullen
Copy link
Contributor Author

Changed behavior to not create TagHelperDescriptors at design time.

public int Property { get; set; }
}

private class MultiHiddenPropertyEditorBrowsableTagHelper : TagHelper
Copy link
Member

Choose a reason for hiding this comment

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

This class has more than one property but how does "MultiHidden" make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More than 1 property on the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove the "Hidden" part of the name.

@dougbu
Copy link
Member

dougbu commented Jul 14, 2015

Implementation looks fine but please test end-to-end behaviour with [EditorBrowsable] on a tag helper class. In particular does Visual Studio provide IntelliSense (for properties of this class) when typing within a services.InitializeTagHelper<NonBrowsableTagHelper>((h, vc) => ...) delegate?

@dougbu
Copy link
Member

dougbu commented Jul 14, 2015

@NTaylorMullen
Copy link
Contributor Author

@dougbu manually tested the comment here: #448 (comment). As for end-to-end testing of this behavior. Can't really validate the design time portion of that; however, the runtime portion will be validated via aspnet/Mvc#2807 since it will be [EditorBrowsable(EditorBrowsableState.Never)].

@dougbu
Copy link
Member

dougbu commented Jul 14, 2015

Not a fan of the ambiguous "Multi" test class names...

@dougbu
Copy link
Member

dougbu commented Jul 14, 2015

:shipit:

- Changed `TagHelperDescriptorFactory` to not create individual descriptors when `EditorBrowsableAttribute` is present and set to `EditorBrowsableState.Never`.
- Added tests to validate the `TagHelperDescriptorFactory` creates the attribute correctly.
- Did not look down the inheritance chain for `EditorBrowsableAttribute` because `TargetElement` is not inherited.

#447
@NTaylorMullen NTaylorMullen force-pushed the nimullen/editorbrowsable.447 branch from 50d798b to b762830 Compare July 16, 2015 21:49
@NTaylorMullen NTaylorMullen merged commit b762830 into dev Jul 16, 2015
@NTaylorMullen NTaylorMullen deleted the nimullen/editorbrowsable.447 branch July 16, 2015 21:51
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