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

Pkellner built in th #2644

Closed
wants to merge 9 commits into from
Closed

Pkellner built in th #2644

wants to merge 9 commits into from

Conversation

pkellner
Copy link
Contributor

@pkellner pkellner commented Feb 1, 2017

Hi @Rick-Anderson This pull requests contains the updates you suggested to AnchorTagHelper as well as doc pages for the Tag Helpers ImageTagHelper and CacheTagHelper. It's all original content. I did not use any content from other sources. Please give me an idea when you plan on reviewing so I can schedule my time to create the rest of the docs. Your feedback is a huge help as I go forward and create the others.

@dnfclas
Copy link

dnfclas commented Feb 1, 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;

@Rick-Anderson
Copy link
Contributor

I'll review it by 10 Feb 2017 - thanks for your contributions

@guardrex
Copy link
Collaborator

guardrex commented Feb 1, 2017

@pkellner You've prob seen that I've sent in consistency PR's here and there. Do you want me to do a pass on it? If not, no worries. I'm just looking to make myself useful around here.

@pkellner
Copy link
Contributor Author

pkellner commented Feb 1, 2017

@guardrex I'm not really the one to talk to here. I'm just helping out with docs. Sorry.

@Rick-Anderson
Copy link
Contributor

@guardrex please review.

@@ -11,7 +11,7 @@ Microsoft has included 17 Tag Helpers that can be immediately used in .net Core

**[AnchorTagHelper](builtin/resources/AnchorTagHelper.md)**

[comment]: **[CacheTagHelper](builtin/resources/AnchorTagHelper.md)**
**[CacheTagHelper](builtin/resources/CacheTagHelper.md)**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the docs and the link text be titled with spaces? It would read better imo Anchor Tag Helper, Cache Tag Helper, etc.

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 would suggest leaving this as is because this is a list of Core Tag Helpers and their actual names. That is, this is not about in general "Cache" Tag Helpers because that might imply it covers the distributed cache tag helper which it does not. Happy to change, just IMHO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Rick-Anderson suggested by following the format of the doc listed in #2644 (comment) that the TH's would be plainly named for reading purposes; but of course, they'll say what they want to do there. I think they read better in plain language.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @Rick-Anderson , could you give a quick opinion on this. @guardrex makes a good point that in the forms doc all the tag helpers are expanded out (Cache Tag Helper, not CacheTagHelper). I always do it the CacheTagHelper way but happy to change all my references, just want to make sure I get it right. -thanks

@@ -8,41 +8,33 @@ By [Peter Kellner](http://peterkellner.net)

The ```AnchorTagHelper``` enhances the html anchor (`<a ... ></a>`) tag. A new set of attributes are defined that work with the anchor tag. The link generated (on the href tag) is based on a combination of these new attributes that work together to form the final redirect URL. That URL can include an optional protocol such as https.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Not that it necessarily hurts anything, but three backticks for codeblocks that break across lines and single backticks for in-text code.
  • "redirect URL" has special meaning ... I think just "URL" there.
  • This passage is a bit confusing. You need prior knowledge or to perhaps read the whole doc to understand that non-standard attributes are converted into standard attributes. I'd refactor this into a complete explanation, and it might even go into the parent document, since it pertains to many of the Tag Helpers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

points 1 and 2 updated.
I've re-read the passage mentioned a few times and it seems OK to me. There is another section already written by Rick that introduced TH's. I don't see anything there that specifically talks about non-standard attributes being converted to standard attributes but I'm not sure that needs to be mentioned (can't hurt to add it there).

My assumption in writing all these built in TH docs it that the reader knows the basics of tag helpers. Valid assumption?

https://docs.microsoft.com/en-us/aspnet/core/mvc/views/tag-helpers/intro

Copy link
Collaborator

@guardrex guardrex Feb 3, 2017

Choose a reason for hiding this comment

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

Yes, that was my point exactly. For my comment here, I may have assumed incorrectly that the reader doesn't know much (or anything) about TH's. That might be the wrong assumption. I can say ... I think this is fair ... that if the reader were to land on this doc and read that sentence ...

The link generated (on the href tag) is based on a combination of these new attributes that work together to form the final redirect URL.

... that they really wouldn't be able to understand what it means.

Whatever the case with all of that, I would remove the word "redirect" from "redirect URL" there at the end, since that has special meaning.

@@ -8,41 +8,33 @@ By [Peter Kellner](http://peterkellner.net)

The ```AnchorTagHelper``` enhances the html anchor (`<a ... ></a>`) tag. A new set of attributes are defined that work with the anchor tag. The link generated (on the href tag) is based on a combination of these new attributes that work together to form the final redirect URL. That URL can include an optional protocol such as https.

For reference, the following ASP.NET ```startup.cs``` and ```SpeakerController.cs``` are defined as you would expect in a default Visual Studio .Net Core Web Project.
For reference, the following ASP.NET ```SpeakerController.cs``` is defined as you would expect in a default Visual Studio .Net Core Web Project.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • ".Net" to ".NET"
  • TH's work outside of VS, so I'd drop mentioning the IDE
  • The SpeakerController.cs file isn't a default project file, so I wouldn't expect to see it 😁 ... but I know what you mean. I think you just need to rephrase it a little. You could just go with the old "The following example shows ... {insert description here of what it shows}:"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh oh. I removed startup.cs from the pull request I believe I made. I'm thinking @guardrex reviewed what was already in the branch approved by @Rick-Anderson and not what was in my latest pull request. I'm guessing I'm not following the right protocol for things to be reviewed. Happy to update my process. LMK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more comment/question here for both @Rick-Anderson and and @guardrex . Should we avoid mentioning Visual Studio? There are lots of mentions of VS in other Core docs. For example Identity: https://docs.microsoft.com/en-us/aspnet/core/security/authentication/identity doesn't need VS but it's mentioned a lot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My wild guess is not mention VS where it isn't explicitly required to do so. MS seems to want to embrace a full spectrum of IDE's that could be used to create ASP.NET Core apps.


## asp-controller
## The attributes are defined as follows
Copy link
Collaborator

@guardrex guardrex Feb 2, 2017

Choose a reason for hiding this comment

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

I suggest instead of a sentence fragment heading to just use a simple section title, such as ## Attributes or ## Anchor Tag Helper attributes ... but the former might be better, since it's understood that the whole doc pertains to this one TH.


- - -

### asp-controller

```asp-controller``` is used to associate which controller will be used to generate the final URL. The only valid choices are controllers that exist in the current project. In our case, to get to a list of all speakers or speaker details you would specify ```asp-controller="Speaker"```. If you only specify ```asp-controller``` and no ```asp-action``` the URL will generate without an error but will likely not be what you expect.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drop the first prepositional phrase and move the comma ... "To get to a list of all speakers or speaker details, you would ..."

If you only specify asp-controller and no asp-action the URL will generate without an error but will likely not be what you expect.

I find the construction a bit awkward ... "... will generate without ..." ... not a common way to phrase this thought in spoken English. Anyway ... the comma ... at least set the phrase off with a comma: "If you only specify asp-controller and no asp-action, the URL will ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made some changes tot he asp-controller section but not sure it's perfect.

## asp-action
- - -

### asp-action

```asp-action``` is the name of the method in the controller that will be included in the final URL. That is, in the example, if the route to the Speaker Detail page is wanted, then the attribute should be set to ```asp-action=Detail```. You should always set ```asp-controller``` when specifying ```asp-action```. If no ```asp-action``` is specified then a URL will generate without an error but will likely not be what you expect.
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is, in the example, if the route to the Speaker Detail page is wanted, then the attribute should be set to asp-action=Detail.

Choppy ... refactor the start of the sentence or break this into two sentences.

If no asp-action is specified then a URL will generate without an error but will likely not be what you expect.

Similar to above: Add a comma after the prepositional phrase and drop "then." Personally, I'd refactor the sentence for something other than the "will generate without" phrase, but that's up to you (and them ... the "real" editors! 😁)

## asp-route-
- - -

### asp-route-

```asp-route-``` is basically 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 you create a tag as follows:
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • "wild card" to "wildcard" in IT contexts
  • I'd add either an asterisk (*) or a hint for the attribute ... either asp-route-* or asp-route-{value}
  • For asp-route-id-="11" below ... no last dash there AFAIK. I think it should be asp-route-id="11". This occurs in a couple of spots here.

Below this line, I see ...

  • href should receive backticks
  • Codeblocks should break across lines
  • The construction of this section seems a bit awkward to me with dangling phrases between code blocks. Personally, I'd clean it up a bit with complete sentences that set off the code examples with colons.
  • "html" to "HTML"

## asp-route
- - -

### asp-route

```asp-route``` provides a way to create a URL that links directly to a named route. Using routing attributes, you can name your routes as shown in the Evaluations method above of the ```SpeakerController```. ```Name = "speakerevals"``` tells the AnchorTagHelper to generate a route directly to that controller method using the URL ```/Speaker/Evaluations```. If ```asp-controller``` or ```asp-action``` is specified in addition to ```asp-route``` then the route generated will not be what you expect. ```asp-route``` should not be used with either of the attributes ```asp-controller``` or ```asp-action``` to avoid a route conflict.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Evaluations with backticks

as shown in the Evaluations method above of the SpeakerController.

Phrasing would be clearer if it were ...

... as shown in the SpeakerController's Evaluations method above.

  • "AnchorTagHelper" ... "Anchor Tag Helper"
  • "... using the URL /Speaker/Evaluations." ... but that's a fragment, so maybe say "using the route /Speaker/Evaluations"

If asp-controller or asp-action is specified in addition to asp-route then the route generated ...

Missing comma and drop "then"

"If asp-controller or asp-action is specified in addition to asp-route, the route generated ..." (comma + drop "then")

will not be what you expect.

The reader is bound to wonder, "What do I expect?" or "What will happen if I do this?" I think the problem should be explained and/or illustrated with an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After researching more, I found reasonable defaults for action and controller methods so I'm updating docs to reflect that. I thought the defaults were not working as I would have expected but I was wrong.

## asp-all-route-data
- - -

### asp-all-route-data

```asp-all-route-data``` allows you to create on your current .net context (that is, the running c# associated with your razor page) a dictionary of key value pairs where the key is the parameter name and the value is the value associated with that key. As the example below shows, you can create an inline dictionary on your razor page and then create the ```AnchorTagHelper``` right after that. You could of course pass in the dictionary with your model or as your model. That avoids having extra c# code on your razor page and gives you better control of the data passed to your view.
Copy link
Collaborator

@guardrex guardrex Feb 2, 2017

Choose a reason for hiding this comment

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

asp-all-route-data allows you to create on your current .net context (that is, the running c# associated with your razor page) a dictionary of key value pairs where the key is the parameter name and the value is the value associated with that key.

  • ".net" to ".NET"
  • "c#" to "C#"
  • "razor page" to "Razor view" ("Razor page" has special meaning; Razor is a proper noun)
  • It's confusing to inject that much parenthetical information (i.e., "that is ...") into the sentence. I think if you explain what the context is first, the sentence will become easier to digest.

You could of course pass in

@tdykstra will be fine with that 😁 (inside joke), but I try to avoid splitting verbs when it isn't necessary. How about just "Of course, you could pass in ..."

... data passed to your view.

Since the data will appear in the view, I wonder if this would go "data passed into your view."

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 think I simplified it. I don't particularly like the design of asp-all-route-data (making it hard to explain). I think the example is more important than the explanation.

## asp-fragment
- - -

### asp-fragment

```asp-fragment``` appends after a hash (```#```) tag at the end of the URL whatever the value assigned to it is. That is, if you create a tag
Copy link
Collaborator

Choose a reason for hiding this comment

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

asp-fragment appends after a hash (#) tag at the end of the URL whatever the value assigned to it is. That is, if you create a tag

asp-fragment defines a URL fragment to append to the URL. The Anchor Tag Helper will add the hash character (#) automatically. If you create a tag:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guardrex I'm confused about some of your changes that look like white space changes. Could you explain a little about how you changed "- - -" and line spacing?


### asp-area

```asp-area``` sets the area name that ASP.NET core will use to set the appropriate route with. Below are examples of how the area attribute causes a remapping of routes. That is, by setting ```asp-area``` to Blogs will invoke the Areas functionality such that the directory Areas/Blogs will prefix the associated controllers and views for this anchor tag.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • "core" to "Core"
  • "... will use to set the appropriate route with." ... prob goes against what they're doing with future tense and "with" at the end of the sentence always gives me a tooth ache [but @tdykstra will like it 😁 (another inside joke 🍺)]. How about "... that ASP.NET Core uses to create the appropriate route to an area."

That is, by setting asp-area to Blogs will invoke the Areas functionality such that the directory Areas/Blogs will prefix the associated controllers and views for this anchor tag.

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
Contributor Author

Choose a reason for hiding this comment

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

nice @guardrex . You said in about 12 words what took me about 30.


```asp-area``` allows for a razor view area to be associated with the hyperlink URL. That is, if you have a project setup in a similar fashion to the project pictured below, the link generated will include as its first segment the area you mention.
* wwwroot
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make wwwroot italics. Same below (I think) for any proper folder or file. Usually, they keep folders and files in italics wherever they appear in docs, including when they appear in lists and tables. @tdykstra will correct me if I'm mistaken.

* Controllers



Specifying an area tag that is valid, such as ```area="Blogs"``` when referencing the ```AboutBlog.cshtml``` file will look like the following using the ```AnchorTagHelper```.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comma after "file".

Again, I'd go with "Anchor Tag Helper" (in plain text) in all of these places where it has to be read.

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'm going with the words separated out as much as I can when I find them.

@@ -130,9 +151,9 @@ The generated HTML will include the areas segment and will be as follows:
> [!TIP]
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Convert this to a > [!NOTE], as they don't want tips.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tips are fine, it's just overuse of them we want to avoid.

@@ -130,9 +151,9 @@ The generated HTML will include the areas segment and will be as follows:
> [!TIP]
> For MVC Areas to work in your web application, the route template must include a reference to the area if it exists. That template (which is the second parameter of the ```routes.MapRoute``` method call) will look as follows: ```template: "{area:exists}/{controller=Home}/{action=Index}"```.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For MVC Areas to work in your web application, the route template must include a reference to the area if it exists. That template (which is the second parameter of the routes.MapRoute method call) will look as follows: template: "{area:exists}/{controller=Home}/{action=Index}".

For MVC areas to work in your web application, the route template must include a reference to the area if it exists. That template, which is the second parameter of the routes.MapRoute method call, will appear as:

template: "{area:exists}/{controller=Home}/{action=Index}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guardrex not sure how to get the template to work in the "[!TIP]" as a separate line.



## asp-protocol
### asp-protocol

Copy link
Collaborator

Choose a reason for hiding this comment

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

... protocol (such as https) in ...

to

... protocol, such as https, in ...

... will look as follows.

to

... will appear as:

@guardrex
Copy link
Collaborator

guardrex commented Feb 3, 2017

I did that once for a note that was placed under a table of values. You can add an asterisk by escaping it with a backslash.

\*

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.

Just two items in this summary. Going back to verify what version this review was against.

@@ -8,41 +8,33 @@ By [Peter Kellner](http://peterkellner.net)

The ```AnchorTagHelper``` enhances the html anchor (`<a ... ></a>`) tag. A new set of attributes are defined that work with the anchor tag. The link generated (on the href tag) is based on a combination of these new attributes that work together to form the final redirect URL. That URL can include an optional protocol such as https.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

points 1 and 2 updated.
I've re-read the passage mentioned a few times and it seems OK to me. There is another section already written by Rick that introduced TH's. I don't see anything there that specifically talks about non-standard attributes being converted to standard attributes but I'm not sure that needs to be mentioned (can't hurt to add it there).

My assumption in writing all these built in TH docs it that the reader knows the basics of tag helpers. Valid assumption?

https://docs.microsoft.com/en-us/aspnet/core/mvc/views/tag-helpers/intro

@@ -8,41 +8,33 @@ By [Peter Kellner](http://peterkellner.net)

The ```AnchorTagHelper``` enhances the html anchor (`<a ... ></a>`) tag. A new set of attributes are defined that work with the anchor tag. The link generated (on the href tag) is based on a combination of these new attributes that work together to form the final redirect URL. That URL can include an optional protocol such as https.

For reference, the following ASP.NET ```startup.cs``` and ```SpeakerController.cs``` are defined as you would expect in a default Visual Studio .Net Core Web Project.
For reference, the following ASP.NET ```SpeakerController.cs``` is defined as you would expect in a default Visual Studio .Net Core Web Project.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh oh. I removed startup.cs from the pull request I believe I made. I'm thinking @guardrex reviewed what was already in the branch approved by @Rick-Anderson and not what was in my latest pull request. I'm guessing I'm not following the right protocol for things to be reviewed. Happy to update my process. LMK.

… helpers in progress. Anchor, Image and Cache.
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 finished incorporating and comment on @guardrex 's edits and added a few of my own for Image, Anchor and Cache Tag Helpers. I've committed my changes to my forked GitHub repo. It looks like those automatically got added to the pull request I made here (#2644). is that true @Rick-Anderson ? In the BuiltIn.md file I changed the word "17" to "many". Is that reflected from my latest work?


- - -

### asp-controller

```asp-controller``` is used to associate which controller will be used to generate the final URL. The only valid choices are controllers that exist in the current project. In our case, to get to a list of all speakers or speaker details you would specify ```asp-controller="Speaker"```. If you only specify ```asp-controller``` and no ```asp-action``` the URL will generate without an error but will likely not be what you expect.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made some changes tot he asp-controller section but not sure it's perfect.

## asp-route
- - -

### asp-route

```asp-route``` provides a way to create a URL that links directly to a named route. Using routing attributes, you can name your routes as shown in the Evaluations method above of the ```SpeakerController```. ```Name = "speakerevals"``` tells the AnchorTagHelper to generate a route directly to that controller method using the URL ```/Speaker/Evaluations```. If ```asp-controller``` or ```asp-action``` is specified in addition to ```asp-route``` then the route generated will not be what you expect. ```asp-route``` should not be used with either of the attributes ```asp-controller``` or ```asp-action``` to avoid a route conflict.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After researching more, I found reasonable defaults for action and controller methods so I'm updating docs to reflect that. I thought the defaults were not working as I would have expected but I was wrong.

## asp-all-route-data
- - -

### asp-all-route-data

```asp-all-route-data``` allows you to create on your current .net context (that is, the running c# associated with your razor page) a dictionary of key value pairs where the key is the parameter name and the value is the value associated with that key. As the example below shows, you can create an inline dictionary on your razor page and then create the ```AnchorTagHelper``` right after that. You could of course pass in the dictionary with your model or as your model. That avoids having extra c# code on your razor page and gives you better control of the data passed to your view.
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 think I simplified it. I don't particularly like the design of asp-all-route-data (making it hard to explain). I think the example is more important than the explanation.

## asp-fragment
- - -

### asp-fragment

```asp-fragment``` appends after a hash (```#```) tag at the end of the URL whatever the value assigned to it is. That is, if you create a tag
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guardrex I'm confused about some of your changes that look like white space changes. Could you explain a little about how you changed "- - -" and line spacing?


### asp-area

```asp-area``` sets the area name that ASP.NET core will use to set the appropriate route with. Below are examples of how the area attribute causes a remapping of routes. That is, by setting ```asp-area``` to Blogs will invoke the Areas functionality such that the directory Areas/Blogs will prefix the associated controllers and views for this anchor tag.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice @guardrex . You said in about 12 words what took me about 30.

@@ -0,0 +1,46 @@
[Back To Built In Tag Helpers List](../../builtin.md)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guardrex not following what to do. Leaving as is. Maybe some fancy link syntax?


The `ImageTagHelper` enhances the html `img` (```<img ... ></img>```) tag. It requires a `src` tag be present as well as the `boolean` attribute `asp-append-version` be included.

If the image source (```src```) is a static file on the host web server, a unique cache bursting string is appended as a parameter to the image source. This insures that if the file on the host web server changes, a unique request url will be generated that includes the updated request parameter. The cache bursting string is a unique value representing the `Sha512` value of the static image file.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

confused on SHA512. See if all over the place in abbreviations. Actually, it's SHA256 so changed that at least. @guardrex (Do I need to mention you since it's your comment?)


If the image source (```src```) is a static file on the host web server, a unique cache bursting string is appended as a parameter to the image source. This insures that if the file on the host web server changes, a unique request url will be generated that includes the updated request parameter. The cache bursting string is a unique value representing the `Sha512` value of the static image file.

If the image source (```src```) is not a static file (possibly a remote url or the file does not exist on the server) then no unique parameter will be generated and the `img` tag will be generated with no additional cache bursting parameter.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guardrex not following "Contractions Needed".

> [!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 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.

DRY


## Additional Resources

* [In Memory Caching](../../../../../performance/caching/memory.md)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guardrex I have a link at the top to the base page (not sure if that is a good plan or not). My guess is people will find these pages on google and not go back to a base page so I think it's best to have the references here. I changed it to "Addional Resource".

Phew. That was a lot of work. Thanks for all the suggestions and fixes. That will help me a lot going forward.

On a logistics note, would it be easier to do pull requests rather than edits for a lot of this kind of stuff? This is my first time using MarkDown and my first time using any of these github management features. Open to ideas to make this have less friction.

@Rick-Anderson
Copy link
Contributor

@pkellner looks complete. I'll make a quick pass.

@dnfclas
Copy link

dnfclas commented Feb 8, 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;

@Rick-Anderson
Copy link
Contributor

@pkellner looks like we need to update the TOC to show these articles. Do you want some help with the TOC?

@pkellner
Copy link
Contributor Author

pkellner commented Feb 8, 2017 via email

@pkellner
Copy link
Contributor Author

pkellner commented Feb 8, 2017 via email

@Rick-Anderson
Copy link
Contributor

@pkellner don't make any commit, I'm working on this.

@pkellner
Copy link
Contributor Author

pkellner commented Feb 8, 2017 via email

@Rick-Anderson
Copy link
Contributor

@pkellner when you say "the content of the Cache Tag Helper is rendered again" do you mean
the content of the Cache Tag Helper is rendered and cached again?

@guardrex
Copy link
Collaborator

guardrex commented Feb 8, 2017

RE: The metadata for the tops of docs appears immediately above the first line. Here's an example from the URL Rewriting doc ...

---
title: URL Rewriting Middleware in ASP.NET Core | Microsoft Docs
author: guardrex
description: An introduction to URL rewriting and redirecting with instructions on how to use URL Rewriting Middleware in ASP.NET Core applications.
keywords: ASP.NET Core, URL rewriting, URL rewrite, URL redirecting, URL redirect, middleware, apache_mod
ms.author: riande
manager: wpickett
ms.date: 12/30/2016
ms.topic: article
ms.assetid: e6130638-c410-4161-9921-b658ce988bd1
ms.technology: aspnet
ms.prod: aspnet-core
uid: fundamentals/url-rewriting
---

Change:

  • title
  • author
  • description - one complete sentence
  • keywords - always start with "ASP.NET Core"
  • date - Use format mm/dd/yyyy
  • asset id - Generate a GUID for this
  • uid - e.g. for the Anchor Tag doc: mvc/views/tag-helpers/builtin/resources/AnchorTagHelper

There are a few spots here and there for a final suggestion. I'll add line notes to the doc.

Check my general comment here: #2644 (comment), as there were a couple of items that I couldn't make line comments to cover.

Copy link
Collaborator

@guardrex guardrex left a comment

Choose a reason for hiding this comment

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

Very nice. Coming together really great. Let me know if any of my remarks are unclear.

@@ -2,16 +2,16 @@

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

Microsoft has included 17 Tag Helpers that can be immediately used in .net Core web projects. These Tag Helpers are complete and production quality and are recommended to be used in projects where appropriate. In this section we will outline all the built in Tag Helpers along with a sample of each tag helper in use.
Microsoft has included multiple Tag Helpers that can be immediately used in .net Core web projects. These Tag Helpers are complete and production quality and are recommended to be used in projects where appropriate. In this section we will outline all the built in Tag Helpers along with a sample of each tag helper in use.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a passage that I couldn't get a line note on the last time. See my general comment on it.

<br/>
[comment]: **[ValidationSummaryTagHelper](builtin/resources/ValidationSummaryTagHelper.md)**

## Additional Resources
Copy link
Collaborator

@guardrex guardrex Feb 8, 2017

Choose a reason for hiding this comment

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

## Additional resources

lowercase "r"


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


The ```AnchorTagHelper``` enhances the html anchor (`<a ... ></a>`) tag. A new set of attributes are defined that work with the anchor tag. The link generated (on the href tag) is based on a combination of these new attributes that work together to form the final redirect URL. That URL can include an optional protocol such as https.
The Anchor Tag Helper enhances the html anchor (`<a ... ></a>`) tag. A new set of attributes are defined that work with the anchor tag. The link generated (on the `href` tag) is based on a combination of these new attributes that work together to form the final URL. That URL can include an optional protocol such as https.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is an important point worth restating: It seems unlikely to me that if a reader lands on this doc out of the blue that they will be able to understand what the 2nd and 3rd sentences mean. A quick example might be able to show this concept.

//...
}
```
The speaker controller used in attribute definitions below is shown here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think most authors are ending these sentences that lead into an example, image, or list with a colon.

//...
}
```
The speaker controller used in attribute definitions below is shown here.

<br/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this is here. Ordinarily, extra lines aren't needed. Perhaps in this case, it is needed, and I don't understand the layout. (In which case, ignore me! 😁)

If the static file exists on the web server, typically in the directory `..wwwroot/images/asplogo.png` the generated html is:

```html
<img
Copy link
Collaborator

Choose a reason for hiding this comment

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

Combine these two lines into one line of code between the two triple-backtick lines ... taking this from four lines to three lines.

src="/images/asplogo.png?v=Kl_dqr9NVtnMdsM2MUg4qthUnWZm5T1fCEimBPWDNgM"/>
```

The value assigned to the parameter `v` is the hash value of the file on disk generated using the .NET library method `CryptographyAlgorithms.CreateSHA256().ComputeHash()`. If the web server is unable to obtain read access to the static file referenced, `wwwroot/images/asplogo.png`, no `v` parameters is added to the `src` attribute, and the original src attribute is unchanged.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Extra space before "If"
  • "parameters is added" ... to "parameters are added"
  • "src" ... to src (single backticks)


### src

To activate the Image Tag Helper, the src attribute is required on the `<img>` element.
Copy link
Collaborator

@guardrex guardrex Feb 8, 2017

Choose a reason for hiding this comment

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

"src" ... to src

To activate the Image Tag Helper, the src attribute is required on the `<img>` element.

> [!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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Stray space in front of "The Image Tag Helper ..."
  • "Cache provider" ... to "cache provider"
  • To match above "Sha512" to "SHA512" ... I still think the spec tho is with a dash (e.g., "SHA-512"), but I certainly could be mistaken on that.
  • Extra space before "The Cache"
  • "The Cache" ... "cache" to lowercase (not a proper noun)

> [!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
Copy link
Collaborator

Choose a reason for hiding this comment

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

## Resources

... and even tho there is only one, it's merely a section header, so I don't think its plurality has to match the number of resources listed. Since I'm a bit of a nutjob when it comes to the "single bullet list" 😢, I'd try to find a second relevant resource to list here. You could list the Response Caching doc and the Response Caching Middleware doc, which are related to the caching concept. Those would be ...

<xref:performance/caching/response>
<xref:performance/caching/middleware>

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

lowercase "r"

@pkellner
Copy link
Contributor Author

pkellner commented Feb 8, 2017 via email

@pkellner
Copy link
Contributor Author

pkellner commented Feb 8, 2017 via email

@pkellner
Copy link
Contributor Author

pkellner commented Feb 8, 2017

Hey @Rick-Anderson . You said hold off on making updates 13 hours ago. @guardrex made some more suggestions including I think how to add the "TOC Block" at the top of each page. Are you (Rick) working on that for me or do you want me to do it. LMK when it's OK to update my pull request again.

@Rick-Anderson
Copy link
Contributor

I'll add the TOC and add "the content of the Cache Tag Helper is rendered and cached again"

@guardrex
Copy link
Collaborator

guardrex commented Feb 8, 2017

Sorry ... I did miss a couple. Most of them require little or no action or refer to general issues. I think I can just summarize and clarify this handful of comments here.

RE: TOC

The TOC ("Table Of Contents") is a different doc. I see he just added a comment on that.


not sure I follow the semi-colon rule on make and model

It's just that when you link two independent clauses into one sentence but where the second sentence opens with a word or clause of its own that requires a comma, the two independent clauses get a semi-colon instead of a comma. For example, we only need a comma for the two independent clauses in ...

Jack fell down and broke his crown, and Jill laughed her ass off.

... but we need a semi-colon for the two independent clauses in ...

Jack fell down and broke his crown; and with great joy, Jill laughed her ass off.

... because the 2nd one starts with a prepositional phrase "with great joy."

☝️ I don't think Mrs. Calvary, my 7th grade English teacher, would approve of those examples 😁, but that's the basic idea.


Yours sounds better but the "NeverRemove" setting cost me a weekend once so I want to save someone else from the same fate. My preference would be to call it "PleaseDontRemoveThisUnlessYouAbsolutelyHaveTo" but guessing that will not happen. Any other suggestions for how to re-word?

My only concern there really is that the other doc seems to imply that NeverRemove will literally result in the entity never being removed ... ever. This seems to say that it might still be removed under memory pressure. I might be reading the other doc wrong. If the other doc is incorrect, then it seems to me the other doc should be modified a hair.


Do I need to mention you since it's your comment?

Nah ... not really. You can just respond without the direct mentions.


Contractions Needed

Just stuff like "do not" to "don't" and "is not" to "isn't" ... contractions


link at the top to the base page

I see what you mean, but it's not a common practice. We'll have to see what the "real editor" 😜 thinks. Remember, I'm just an "unreal editor" ... I don't exist. 😁 That was a good one. 😄


On a logistics note, would it be easier to do pull requests rather than edits for a lot of this kind of stuff?

Not sure, but that sounds like a good idea to batch up the commits for each set of changes. They haven't yelled at me (yet 😁) for not using a 2nd branch to batch up changes for the PR branch.

On a separate note, I hazard a guess that it's easier to work on these when there is one PR per doc and the docs are handled separately. They'll say whether or not they want one PR for each different doc.

As for the rest, I'm learning as well. This was my first time with the GH review feature.


My assumption in writing all these built in TH docs it that the reader knows the basics of tag helpers. Valid assumption?

idk on that one. I think there were just a couple of spots where just a few more sentences might ensure that if someone landed on the doc cold that they wouldn't get lost in the language.


Could you explain a little about how you changed "- - -" and line spacing?

Not sure what you mean by "how you changed" ... maybe they made changes? I think you fixed the vast majority of those spots where I saw extra line spaces, and I think I list in last night's comments any others that I saw.

On the use of the horizontal rule, I'm not sure what they prefer. It isn't common to use them to separate sections in docs where a section heading is present. It's just one blank line ... the heading ## Blah blah blah in sentence case ... and another blank line before getting to the section content.


When I preview using docx, the table does not show with formatting. I have no idea if I did it right. I changed the true,false to two separate lines like in other places.

Not sure ... but I build them with no extra spaces, three dashes only, and frequently leave off the opening and closing bars (but that's optional ... you can have them if you like). For that one table, I'd put true and false in code.

Attribute Type | Example Value
--- | ---
boolean | `true`/`false`

or

| Attribute Type | Example Value |
| --- | --- |
| boolean | `true`/`false` |

If you want to center (or right-justify a column), use the colon:

Attribute Type | Example Value
--- | :---:
boolean | Centered `true`/`false`
Attribute Type | Example Value
--- | ---:
boolean | Right-justified `true`/`false`

I've also had some good luck with paragraphs where I need to separate a description from the setting values.

Attribute Type | Example Value
--- | ---
boolean | <p>This attribute blah blah blah whatever.</p><p>`true`/`false`</p>

Renders as ...

Attribute Type Example Value
boolean

This attribute blah blah blah whatever.

true/false


"Additional resources" to "Resources"

This was a change idea rather recent in the docs. I think the plan is to get them changed over slowly and have new docs just use ## Resources.

They decided on Additional resources or Next steps as appropriate for the content.

@pkellner
Copy link
Contributor Author

pkellner commented Feb 8, 2017

@Rick-Anderson @guardrex I'm confused on this pull request process. Doesn't Rick need to merge this pull request into the pkellner-th-branch first, then make the changes? (then I can pull back to my fork and make more changes). Please correct me if there is a better way. It seems to me that we should make a branch underneath pkellner-th-branch for each remaining tag helper (happy to make the list) and then manage each separately. Then, we can close this pull request and as each of the other th's are completed, they can be merged into this branch (pkellner-th-branch). Then, eventually, after all the th's are done, pkellner-th-branch can be merged into the production one.

@Rick-Anderson
Copy link
Contributor

@pkellner at this point I think it's easier to close:

This PR diverged from master so much I couldn't create a TOC.
Does that work?

@Rick-Anderson
Copy link
Contributor

Once we get #2698 merged updates will be much smoother/quicker. Thanks for all your efforts.

@pkellner
Copy link
Contributor Author

pkellner commented Feb 9, 2017 via email

@guardrex
Copy link
Collaborator

guardrex commented Feb 9, 2017

@pkellner

Process

  1. Update my master: git fetch upstream, git checkout master, git rebase upstream/master, git push -f origin master. [Note: "upstream" was added as a remote initially with git remote add upstream http://github.com/aspnet/Docs.git.]
  2. Branch to start a new doc (or fix something) in the web portal
  3. Work on that branch in the web portal and git pull locally to build for display locally docfx -t default --serve
  4. PR it when ready
  5. Commit to the branch in the web UI throughout the review process until the PR is approved

Considerations

  • Working directly on the branch attached to the PR might not be preferred for them, but they haven't threatened 👦 🔫 to shoot me yet. 😄
  • Working in the web UI has been fine for me; however, I might switch over to working locally and then syncing my local branch with GH. This would help reduce my many commits.

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.

5 participants