Skip to content

Blazor API Review: Built-in Components #12548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rynowak opened this issue Jul 25, 2019 · 7 comments
Closed

Blazor API Review: Built-in Components #12548

rynowak opened this issue Jul 25, 2019 · 7 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed

Comments

@rynowak
Copy link
Member

rynowak commented Jul 25, 2019

Summary

We want to make sure we're happy with the naming and consistency of all of our built-in components since these names get locked forever. Generally these components have gotten lots of usage so there's not much to say bad about their functionality.

Some of these are covered in their own issues (Auth, Forms, Router).

Cascading Value

namespace Microsoft.AspNetCore.Components
{
    public partial class CascadingValue<T> : Microsoft.AspNetCore.Components.IComponent
    {
        public CascadingValue() { }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public Microsoft.AspNetCore.Components.RenderFragment ChildContent { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public bool IsFixed { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public string Name { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public T Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        public void Attach(Microsoft.AspNetCore.Components.RenderHandle renderHandle) { }
        public System.Threading.Tasks.Task SetParametersAsync(Microsoft.AspNetCore.Components.ParameterCollection parameters) { throw null; }
    }
}

Layout

namespace Microsoft.AspNetCore.Components
{
    public abstract partial class LayoutComponentBase : Microsoft.AspNetCore.Components.ComponentBase
    {
        protected LayoutComponentBase() { }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public Microsoft.AspNetCore.Components.RenderFragment Body { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
    }
}

PageDisplay

namespace Microsoft.AspNetCore.Components
{
    public partial class PageDisplay : Microsoft.AspNetCore.Components.IComponent
    {
        public PageDisplay() { }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public Microsoft.AspNetCore.Components.RenderFragment AuthorizingContent { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public Microsoft.AspNetCore.Components.RenderFragment<Microsoft.AspNetCore.Components.AuthenticationState> NotAuthorizedContent { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public System.Type Page { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        [Microsoft.AspNetCore.Components.ParameterAttribute]
        public System.Collections.Generic.IDictionary<string, object> PageParameters { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
        public void Attach(Microsoft.AspNetCore.Components.RenderHandle renderHandle) { }
        public System.Threading.Tasks.Task SetParametersAsync(Microsoft.AspNetCore.Components.ParameterCollection parameters) { throw null; }
    }
}
@rynowak rynowak added area-blazor Includes: Blazor, Razor Components API Review labels Jul 25, 2019
@rynowak rynowak added this to the 3.0.0-preview9 milestone Jul 25, 2019
@rynowak rynowak self-assigned this Jul 25, 2019
@danroth27 danroth27 mentioned this issue Jul 25, 2019
20 tasks
@rynowak
Copy link
Member Author

rynowak commented Jul 25, 2019

I only have a few comments here because I feel like this stuff is in pretty good shape.

For CascadingValue<> should we try to make this consistent with CascadingAuthenticationState? These have different naming, but IIRC CascadingAuthenticationState is just a specialized CascadingValue<>. Should we adopt a pattern? Should this inherit?

For LayoutComponentBase we called the content Body - but we call this ChildContent in lots of place. Should we make this be called ChildContent? I don't feel like changing is super important. Would being consistent with other stuff help us explain these concepts?

I'm a little worried about how coupled PageDisplay appears to be with authorization - but I supposed we can talk about this more during the routing review.

@BANG88
Copy link

BANG88 commented Jul 25, 2019

How can I create a Component that inherit from a built-in Component.

MyInput use the Input's parameters and I can define the other parameter I need.

@rynowak
Copy link
Member Author

rynowak commented Jul 25, 2019

You can inherit from another class in razor using @inherits

@inherits SomeOtherClass

@SteveSandersonMS
Copy link
Member

For CascadingValue<> should we try to make this consistent with CascadingAuthenticationState? These have different naming, but IIRC CascadingAuthenticationState is just a specialized CascadingValue<>. Should we adopt a pattern? Should this inherit?

The naming seems reasonable consistent to me already. Maybe I'm too close to it and am missing something. What's inconsistent here?

The original name we wanted was AuthenticationStateProvider, which I still think is a better name, but unfortunately that's also the name for the corresponding DI service.

I'm definitely open to name changes here if there's another proposal.

Should this inherit?

I don't think so. CascadingValue lets users supply their own value, whereas CascadingAuthenticationState doesn't (it controls the value that gets supplied). Is that the inconsistency you're getting at?

@SteveSandersonMS
Copy link
Member

For LayoutComponentBase we called the content Body - but we call this ChildContent in lots of place. Should we make this be called ChildContent? I don't feel like changing is super important. Would being consistent with other stuff help us explain these concepts?

ChildContent is a very generic name that doesn't imply anything in particular about how the content will be used, whereas Body is saying specifically what role the content plays.

I'd certainly consider naming this BodyContent, especially if we're making a convention that these things all end with Content (e.g., NotAuthorizedContent, etc.).

We're also inconsistent in Router vs AuthorizeView, where Router has NotAuthorizedContent but AuthorizeView has NotAuthorized.

@SteveSandersonMS
Copy link
Member

I'm a little worried about how coupled PageDisplay appears to be with authorization - but I supposed we can talk about this more during the routing review.

Agreed. We did propose a different layering but quickly vetoed it on the grounds that we didn't think it helped end users very much. Happy to reconsider, though time is pretty short for this kind of change.

@rynowak
Copy link
Member Author

rynowak commented Jul 25, 2019

We want to rename all of the render fragment parameters to drop the Content suffix on PageDisplay

rynowak pushed a commit that referenced this issue Jul 28, 2019
Fixes: #12548

Renaming properties to drop 'Content' as a suffix. We haven't been
consistent in using this, and we're removing it instead of adding it
elsewhere.
rynowak pushed a commit that referenced this issue Jul 30, 2019
Fixes: #12548

Renaming properties to drop 'Content' as a suffix. We haven't been
consistent in using this, and we're removing it instead of adding it
elsewhere.
rynowak pushed a commit that referenced this issue Jul 30, 2019
Fixes: #12548

Renaming properties to drop 'Content' as a suffix. We haven't been
consistent in using this, and we're removing it instead of adding it
elsewhere.
@rynowak rynowak added Done This issue has been fixed and removed Working labels Jul 30, 2019
@rynowak rynowak closed this as completed Jul 30, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

3 participants