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

Ra p kellner built in th #2785

Merged
merged 16 commits into from
Jul 8, 2017
Merged

Ra p kellner built in th #2785

merged 16 commits into from
Jul 8, 2017

Conversation

pkellner
Copy link
Contributor

@pkellner pkellner commented Feb 16, 2017

I've made small changes as noted in the commit comments. I believe this is ready for review by @Rick-Anderson

fixes #2256 and fixes #102

Internal review URL

… assetid by incrementing 1. Renamed all Additional Resources to Resources per @Devguard, fixed incorrect expires-sliding code example, removed warning about Cache NeverRemove.
@dnfclas
Copy link

dnfclas commented Feb 16, 2017

Hi @pkellner, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@@ -182,7 +182,7 @@ The domain in the example is localhost, but the Anchor Tag Helper uses the websi

- - -

## Additional Resources
## Resources
Copy link
Collaborator

Choose a reason for hiding this comment

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

They had a meeting and decided not to use "Resources" after all, but the "r" should be lowercase.

## Additional resources

# ImageTagHelper
title: Anchor Tag Helper | Microsoft Docs
author: Peter Kellner
description: Shows how to work with Image Tag Helper
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will become the Twitter description and the meta description, so imo it should be a complete sentence with punctuation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rick-Anderson put in this description line so leaving as is for now. It's the same in all three tag helpers.

@@ -1,4 +1,18 @@
# ImageTagHelper
title: Anchor Tag Helper | Microsoft Docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Anchor 👉 Image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add three dashes for the first line ...

---
title: Image Tag Helper | Microsoft Docs
... continued ...

ms.prod: aspnet-core
uid: mvc/views/tag-helpers/builtin-th/ImageTagHelper
---

Copy link
Collaborator

Choose a reason for hiding this comment

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

Kill the blank line here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

killing the blank line in all 3 tag helpers

The Cache Tag Helper is dependent on the the [memory cache service](xref:performance/caching/memory). The Cache Tag Helper adds the service if it has not been added.

## Additional Resources
## Resources
Copy link
Collaborator

Choose a reason for hiding this comment

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

## Additional resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in all three tag helpers

@@ -40,7 +54,7 @@ To activate the Image Tag Helper, the src attribute is required on the `<img>` e
> [!NOTE]
> The Image Tag Helper uses the `Cache` provider on the local web server to store the calculated `Sha512` of a given file. If the file is requested again the `Sha512` does not need to be recalculated. The Cache is invalidated by a file watcher that is attached to the file when the file's `Sha512` is calculated.

## Additional Resource
## Resources
Copy link
Collaborator

Choose a reason for hiding this comment

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

## Additional resources

…ces", put back in warning about NeverRemove based on feedback from team: #2698 (comment)
@pkellner
Copy link
Contributor Author

I've made fixes as suggested by @guardrex and added back the NeverRemove warning for Cache Tag Helper. Ready for review @Rick-Anderson

@guardrex
Copy link
Collaborator

@Rick-Anderson put in this description line so leaving as is for now. It's the same in all three tag helpers.

He's probably testing ConsistencyRex:tm: to make sure I'm still on the ball! :grin:

It'll show up as the page description in search results and when Tweeted. IMHO they should be nice.

@Rick-Anderson
Copy link
Contributor

@pkellner change the title page to something like:

ASP.NET Core MVC include many [Tag Helpers](xref: mvc/views/tag-helpers) that can help you be more productive in creating Razor markup. This document provides link to many of these built-in Tag Helpers.

@Rick-Anderson
Copy link
Contributor

The speaker controller used in attribute definitions below is shown here.+

The speaker controller below is used in this document.

@Rick-Anderson
Copy link
Contributor

@pkellner I did some clean up to the anchor TH. There are a bunch of internal comments I left. Can you pull down the latest commit , address the comments and submit?

@pkellner
Copy link
Contributor Author

pkellner commented Feb 18, 2017 via email

@pkellner
Copy link
Contributor Author

@Rick-Anderson I'm baffled on how to get your comments across to my fork so I can work on them. Am I meant to just look at them and then put them into my branch manually or is there someway to incorporate the change suggestions? I can see them on github in the comments.

Copy link
Contributor Author

@pkellner pkellner left a comment

Choose a reason for hiding this comment

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

I've re-worked asp-route-* quite a bit and added more details and examples (including using model instead of hard coding id). I believe this attribute is going to be the cause of a lot of bug ridden code but it is what it is. My feeling is the extra explanation is necessary. I'm open to suggestions on how to be more concise. @Rick-Anderson and @guardrex are awesome at taking my many words and bringing them down to few that mean the same thing. I appreciate that and will likely get better over time (or hopefully).

@pkellner
Copy link
Contributor Author

@Rick-Anderson I notice that when I search "Anchor Tag Helper" with the docfx running localhost I get the correct page. When I type AnchorTagHelper (which is what I think people will search) I don't get any result. How to add that to the page index?

@guardrex
Copy link
Collaborator

Theory only ... I would hope that it can be added to the keywords in the metadata. I'm not aware of whether or not the DocFX search uses meta keywords tho. It's a good question.

…er to be more explicit about defaults as well as say "href" instead of "final URL".
… routing parameters so I updated the doc to reflect that.
@Rick-Anderson
Copy link
Contributor

local DocFX might not find it. Put it in the keywords metadata.

@pkellner
Copy link
Contributor Author

@Rick-Anderson just wanted to make sure you know that I'm waiting on you for review on this (I think).

@Rick-Anderson
Copy link
Contributor

@pkellner sorry, yes. I'm really swamped right now getting out Dev15. Ping me in a couple weeks.

@pkellner
Copy link
Contributor Author

pkellner commented Mar 3, 2017

hey @guardrex , I know the team is heads down for vs2017 right now and wondering if you can help me. I've discovered that my documentation for Cache Tag Helper, attribute vary-by-route is wrong (as well as what everyone else on the web seems to think). I'd like to debug it but have not been able to get the mvc source to build for a long time. I noticed you had some comments in that regard. Besides downloading the source, downloading a 2017RC and opening the mvc.sln and building it, are there any tricks everyone but me knows? I get stuck on the error: RuntimeIdentifier must be set for .NETFramework executables. Consider RuntimeIdentifier=win7-x86 or RuntimeIdentifier=win7-x64 BTW, I posted my issue here but no takers (http://stackoverflow.com/questions/42563832/why-does-asp-net-core-cachetaghelper-vary-by-route-not-refresh-cache-on-route-ch)

@guardrex
Copy link
Collaborator

guardrex commented Mar 3, 2017

@pkellner Yeah, if you're compiling for .NET Framework, it's demanding a ...

<RuntimeIdentifiers>win7-x86</RuntimeIdentifiers>

... in the csproj properties.

If the app in question has netcoreapp1.x target framework and .NET Framework (e.g., net451) ...

<TargetFrameworks>net451;netcoreapp1.1</TargetFrameworks>

... then you'll need to qualify your RID this way ...

<RuntimeIdentifier Condition=" '$(TargetFramework)' == 'net451' ">win7-x86</RuntimeIdentifier>

The qualifier ... indeed! the whole RID RuntimeIdentifier itself if I'm reading their issues correctly ... will go away when the CLI gets a little "smarter" and is able to infer the RID all by itself. I don't think that work is quite done yet. dotnet/sdk#396 ... if it is done, it will be with nightly bits.

There's also a reference for System and Microsoft.CSharp that I was prompted to add from team samples.

Check out the Response Caching Sample for a pattern you can follow: https://github.com/aspnet/Docs/blob/csproj/aspnetcore/performance/caching/middleware/sample/ResponseCachingSample.csproj

That's over on the csproj branch ... I don't think you'll see it yet on the public docs.

[EDIT] I'm sooo glad you said something. There's a little bug there in the sample I need to fix really quick. 😉 Double RID lines. I'll get a PR in tonight to square it away. [EDIT] The PR is in to remove that first <RuntimeIdentifiers> line in the sample I link.

@Rick-Anderson
Copy link
Contributor

@pkellner @guardrex I pinged the team on your SO question.

@guardrex
Copy link
Collaborator

guardrex commented Mar 4, 2017

@pkellner Did that <RuntimeIdentifier> info ☝️ help?

If you update your tooling (perhaps to nightly), you might see the CLI "smart enough" now to infer the RID correctly for a .NET Framework app. If so, you should be able to drop the line entirely. I intend to do some testing with nightly bits soon-ish to confirm the behavior. I have one other little thingy I need to do this weekend. If I square that away, I might be able to give you an update by Sunday night.

@pkellner
Copy link
Contributor Author

pkellner commented Mar 4, 2017

@guardrex I did not try the build again but would love to get it working locally. @NTaylorMullen answered my question on SO so the immediate need passed. Bump me when you've done your test and I'll grab a nightly and build on my end (I assume it requires 2017 to work or does it work with 2015 also?). Thanks

@pkellner
Copy link
Contributor Author

Hey @Rick-Anderson , been sick all week but mostly better now and hoping to work on rest of built-in tag helpers. LMK when you can what is next for me.


`<a asp-controller="Speaker" asp-action="Detail" asp-route-id-="11">Speaker 11</a>`
and have the default route template defined in your **startup.cs** as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Files are italic, please check all ** and make sure you have the right styling.

Copy link
Member

Choose a reason for hiding this comment

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

There should also be a capital "S" on "startup.cs".


This is because a route was found that matched a single parameter "id" in the ```SpeakerController``` method ```Detail```. If there was no parameter match, say for example you created the tag helper
The **cshtml** file that contains the Anchor Tag Helper necessary to use the **speaker** model parameter passed in from the controller to the view is as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

no style on cshtml

speaker should be code fenced, not bold. Do you ever show the speaker model?

@@ -1,6 +1,6 @@
---
title: Anchor Tag Helper | Microsoft Docs
author: Peter Kellner
title: Cache Tag Helper | Microsoft Docs
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to
title: Cache Tag Helper in ASP.NET Core MVC
remove | Microsoft Docs

@Rick-Anderson
Copy link
Contributor

@pkellner review 2b539f8 which is the changes I made. Let me know if I made any mistakes. Let's get this thing published.

@Rick-Anderson
Copy link
Contributor

@pkellner don't make any changes to DistributedCacheTagHelper.md until I update it.

<a asp-controller="Speaker" asp-action="Detail" >Speaker Detail</a>
```

The generated URL will be
Copy link
Collaborator

@guardrex guardrex Jun 30, 2017

Choose a reason for hiding this comment

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

@Rick-Anderson Passing quick ?: Are the style manual guidelines on present/past tense somewhat relaxed for the two docs repos? You know, they warn off of will, was, and verbs ending in -ed. However, it's not uncommon to see the tenses change when writing in a conversational tone and telling the reader that 'this will happen', especially in tutorial/walk-through topics. I noticed that reviews don't necessarily call tense out here and over on .NET Core docs, so just wondering if that was a relaxed provision?

@Rick-Anderson
Copy link
Contributor

@pkellner @guardrex Great point. Avoid future tense.
The generated URL

@guardrex
Copy link
Collaborator

lol ... apparently it's 🥊 Gang up on Peter day! 🥊 ...... (He knows we care. 👍)

I search my new content on was, will, were, can (looking for "you can" language that I can refactor), and ed. The will ones can usually go to is and are. For some of them, you can get a really nice action verb from what's already there. Example:

The following example will cache the contents of the Cache Tag Helper until 5:02 PM on January 29, 2025.

to

The following example caches the contents of the Cache Tag Helper until 5:02 PM on January 29, 2025.

@pkellner
Copy link
Contributor Author

@guardrex @Rick-Anderson No worries about me jumping on it (until I get the word). I'm just still hoping I remember how Cache works (and markup for that matter).

# Anchor Tag Helper

By [Peter Kellner](http://peterkellner.net)

The Anchor Tag Helper enhances the html anchor (`<a ... ></a>`) tag by adding new attributes. The link generated (on the `href` tag) is created using the new attributes. That URL can include an optional protocol such as https.
Copy link
Member

Choose a reason for hiding this comment

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

"html" should be all caps

<a href="/Speaker">All Speakers</a>
```

If the `asp-controller` is specified and `asp-action` is not, the default `asp-action` will be the default controller method of the currently executing view. That is, in the above example, if `asp-action` is left out, and this Anchor Tag Helper generated from the Home index view (**/Home**), the generated markup will be:
Copy link
Member

Choose a reason for hiding this comment

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

There's an extra space before "That is,".


- - -

### asp-route-{value}

`asp-route-` is a wild card route prefix. Any value you put after the trailing dash will be interpreted as the parameter to pass into the route. For example, if a tag is created as follows:
`asp-route-` is a wild card route prefix. Any value you put after the trailing dash will be interpreted as a potential route parameter. If a default route is not found, this route prefix will be appended to the generated href as a request parameter and value. Otherwise it will be substituted in the route template.
Copy link
Member

@scottaddie scottaddie Jun 30, 2017

Choose a reason for hiding this comment

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

There's an extra space before "Otherwise".

Copy link
Collaborator

Choose a reason for hiding this comment

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

... and the adverb "Otherwise" in that position gets a comma.


you would get generated the html
The generated html will then be as follows because **id** was found in the default route.
Copy link
Member

Choose a reason for hiding this comment

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

"html" should appear in all caps


As the example below shows, an inline dictionary is created and the data is passed to the razor view. The data could also be passed in with your model to keep the Razor view simpler.
As the example below shows, an inline dictionary is created and the data is passed to the razor view. As an alternative, the data could also be passed in with your model.
Copy link
Member

Choose a reason for hiding this comment

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

The "r" in "razor" should be capitalized, since it's a proper noun.


```
http://localhost/Speaker/EvaluationsCurrent?speakerId=11&currentYear=true
```

When the link is clicked, this will call the controller method `EvaluationsCurrent` because that controller has two string parameters that match what has been created from the `asp-all-route-data` dictionary.
When the link is clicked, the controller method `EvaluationsCurrent`is called: It is called because that controller has two string parameters that match what has been created from the `asp-all-route-data` dictionary.
Copy link
Member

Choose a reason for hiding this comment

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

Replace colon after 1st sentence with a period.

@@ -125,7 +199,7 @@ Hash tags are useful when doing client side applications. They can be used for e

### asp-area

`asp-area` sets the area name that ASP.NET Core uses to set the appropriate route. Below are examples of how the area attribute causes a remapping of routes. Setting `asp-area` to Blogs prefixes the directory Areas/Blogs to the routes of the associated controllers and views for this anchor tag..
`asp-area` sets the area name that ASP.NET Core uses to set the appropriate route. Below are examples of how the area attribute causes a remapping of routes. Setting `asp-area` to Blogs prefixes the directory `Areas/Blogs` to the routes of the associated controllers and views for this anchor tag.
Copy link
Member

Choose a reason for hiding this comment

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

There's an extra space before "Setting".

@@ -12,15 +12,14 @@ ms.technology: aspnet
ms.prod: aspnet-core
uid: mvc/views/tag-helpers/builtin-th/CacheTagHelper
---

# Cache Tag Helper
# Cache Tag Helper in ASP.NET Core MVC

By [Peter Kellner](http://peterkellner.net)


The Cache Tag Helper provides the ability to dramatically improve the performance of your ASP.NET Core app by caching its content to the internal ASP.NET Core cache provider.
Copy link
Member

Choose a reason for hiding this comment

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

There's an extra space before "Cache".

@pkellner
Copy link
Contributor Author

Hey @scottaddie . Can you do updates instead of comments? I'm bad at markup and small changes seem to often turn into +1 Fix, -2 Break.

@scottaddie
Copy link
Member

@pkellner Not a problem. I'm on it!

@pkellner
Copy link
Contributor Author

pkellner commented Jul 1, 2017

@Rick-Anderson @guardrex @scottaddie uh-oh. I may have screwed up. I thought my fork was hopelessly behind so I removed it and reforked. Is my fork where the changes were? If so, do you have a copy of those changes? Help!

Copy link
Contributor Author

@pkellner pkellner left a comment

Choose a reason for hiding this comment

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

I'm good with all the changes. Left 1 comment on vary-by-user.

@pkellner
Copy link
Contributor Author

pkellner commented Jul 3, 2017

I gather it's not an issue that I deleted my repo (https://help.github.com/articles/checking-out-pull-requests-locally/#modifying-an-inactive-pull-request-locally) @Rick-Anderson @guardrex @scottaddie . Do you have what you need to commit all the built-in tag helpers I've done? That is, Anchor,Cache,DistributedCache,Environment and Image? My thought would be to get those all wrapped and checked in and then I'll re-fork and finish all the form built-in tag helpers I've not yet done. Make sense? other thoughts?

@pkellner
Copy link
Contributor Author

pkellner commented Jul 8, 2017

@Rick-Anderson Be great to start working on the docs for built-in-taghelpers again. I know you are swamped with other stuff but if you could find some time to get me back on track and commit the TH's I've done (or LMK what I need to do), that would be appreciated.

@Rick-Anderson Rick-Anderson merged commit 1836273 into dotnet:ra-pKellner-built-in-TH Jul 8, 2017
@Rick-Anderson
Copy link
Contributor

@pkellner it should be live in 8 hours. Did you have a question on vary-by-user?

@Rick-Anderson
Copy link
Contributor

@pkellner actually I'll get this into master/live monday

@pkellner
Copy link
Contributor Author

pkellner commented Jul 8, 2017

@Rick-Anderson on vary-by-user, my comment was in response to a question posted saying "it was not clear". I've forked the docs and created a pull request that fixes vary-by-user as well as one casing error (razor -> Razor).

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.

6 participants