Skip to content
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

Razor Pages Filters #5835

Merged
merged 31 commits into from
Apr 12, 2018
Merged

Razor Pages Filters #5835

merged 31 commits into from
Apr 12, 2018

Conversation

Rick-Anderson
Copy link
Contributor

@Rick-Anderson Rick-Anderson commented Mar 30, 2018

@Rick-Anderson Rick-Anderson requested a review from pranavkm March 30, 2018 01:44
@Rick-Anderson Rick-Anderson changed the title WIP Razor Pages Filters Razor Pages Filters Apr 5, 2018


<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore.App" Version="2.1.0-preview2-30457" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just point this to 2.1.0-preview2-final (when it's available, which is likely soon)?


[!code-csharp[Main](filter/sample/PageFilter/StartupSync.cs?name=snippet2&highlight=11)]

## Implement Razor Page filters by overriding page handlers
Copy link
Contributor

Choose a reason for hiding this comment

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

overriding filter methods (not handlers)

@pranavkm
Copy link
Contributor

pranavkm commented Apr 5, 2018

Yet another thing, that's missing is specifying filters on PageModel instances. Maybe we could trim out the async version of overriding and show a filter attribute being applied?

@Rick-Anderson
Copy link
Contributor Author

@pranavkm can you send me the code for a filter attribute?

@pranavkm
Copy link
Contributor

pranavkm commented Apr 5, 2018

@Rick-Anderson
Copy link
Contributor Author

@fmaeseele please review.

@Rick-Anderson
Copy link
Contributor Author

@pranavkm Can you take another look. It should be ready.

</ItemGroup>

<ItemGroup>
<Content Update="Pages\About.cshtml">
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

_value = value;
}

public override async Task OnResultExecutionAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to public override void OnResultExecuting(ResultExecutingContext context)`? Would be simpler than the async overload.

@@ -0,0 +1,8 @@
@page
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional? The folder name \ file name seem a bit suspect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted about2. I don't need it to show:

services.AddMvc()
   .AddRazorPagesOptions(options =>
   {
       options.Conventions.AddFolderApplicationModelConvention(
           "/subFolder",
           model => model.Filters.Add(new SampleAsyncPageFilter(_logger)));
   });


public static IWebHostBuilder CreateWebHostBuilder(string[] args) =>
WebHost.CreateDefaultBuilder(args)
// .UseStartup<StartupSync>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Cleanup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I formatted the commented out code but I need all 3 startup class references to test the 3 startup classes - depending on what I want to show.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okies

@@ -0,0 +1,53 @@
using Microsoft.AspNetCore.Builder;
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

{
options.Conventions.AddFolderApplicationModelConvention(
"/subFolder",
model => model.Filters.Add(new SampleAsyncPageFilter(_logger)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there value in showing a sync and async filter being added globally? I'd pick one as long as we show a sample of a sync filter and an async filter being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sample is to show AddFolderApplicationModelConvention
In the rendered document, this async filter is shown in the previous snippet.

@@ -0,0 +1,37 @@
/* Please see documentation at https://docs.microsoft.com/aspnet/core/client-side/bundling-and-minification
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, do we always include all the css \ scripts with these samples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lost that battle long ago. I used to exclude them with .gitignore and require you to manually override the ignore file. Now I ask folks to manually exclude them which is usually forgotten.

@@ -246,7 +249,6 @@
### [Build Docker images](/dotnet/articles/core/docker/building-net-docker-images)
### [Visual Studio Tools for Docker](xref:host-and-deploy/docker/visual-studio-tools-for-docker)
### [Publish to a Docker image](https://azure.microsoft.com/documentation/articles/vs-azure-tools-docker-hosting-web-apps-in-docker/)
## [Proxy and load balancer configuration](xref:host-and-deploy/proxy-load-balancer)
Copy link
Contributor

Choose a reason for hiding this comment

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

?

@Rick-Anderson Rick-Anderson merged commit 288347e into master Apr 12, 2018
@Rick-Anderson Rick-Anderson deleted the rpFilter/ra branch April 12, 2018 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants