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

Revamp Permalink section #5912

Merged
merged 3 commits into from
Apr 4, 2017
Merged

Revamp Permalink section #5912

merged 3 commits into from
Apr 4, 2017

Conversation

rlue
Copy link
Contributor

@rlue rlue commented Feb 28, 2017

The previous version contained errors regarding the definition and usage of the :path permalink template variable (see Issue #5901). This version corrects those errors, removes redundant language where possible, and restructures the section to place examples before template variable definitions.

The reasoning behind the third change is that it's generally easier for learners to understand a generalized or abstracted definition after having seen concrete examples in action first.

The previous version contained errors regarding the definition and usage of the `:path` permalink template variable. This version corrects those errors, removes redundant language where possible, and restructures the section to place examples before template variable definitions.

The reasoning behind the third change is that it's generally easier for learners to understand a generalized or abstracted definition after having seen concrete examples in action first.
@parkr
Copy link
Member

parkr commented Mar 2, 2017

/cc @jekyll/documentation

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

Much clearer indeed, thanks for this useful addition to the docs ✍️

```
├── _apidocs
│   └── archive
│   └── deprecated.md
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you have "deprecated.md" as an example of the file here? What's this about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, I was worried that might raise an eyebrow.

I figured that a generic-but-realistic use case would make for a more descriptive example than apidocs/mydocs/doc1.md; i.e., that it would be easier to imagine why you might choose to set your permalinks up as, say, doc/deprecated.html than awesome/doc1.html. But I also wrote it in a hurry, and I recognize that maybe apidocs/archive/deprecated.md is not as realistic as it could be.

If it creates more confusion than it resolves, I'm happy to change it back to apidocs/mydocs/doc1.md and resubmit the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using a generic file name that doesn't call attention to itself like "deprecated" does. Maybe myfile.my or something. I have some more comments that I'll insert inline.

Copy link
Member

Choose a reason for hiding this comment

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

We could use a more generic file name, but I don't mind the current example here, seems pretty explanatory to me.

@@ -89,18 +89,59 @@ choice and written out to `<dest>/my_collection/some_subdir/some_doc.html`.

## Configuring permalinks for collections {#permalinks}

You can customize the [Permalinks](../permalinks/) for your collection's documents by setting `permalink` property in the collection's configuration as follows:
You can specify a pattern for the URLs where your Collection documents will reside with the [`permalink` property](../permalinks/):
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a modifier problem with the last "with the ..." clause. Maybe recast as "Through the permalink property, you can specify a pattern for the URLs for your Collection documents. The permalink also defines the structure for the files in the output."

@tomjoht
Copy link
Contributor

tomjoht commented Mar 3, 2017

In the Examples section, the tree formatting is messed up. This may be intentional, but it looks awkward like this to me. Can you preserve the tree formatting in the examples in a way that leaves the tree diagrams complete?

@tomjoht
Copy link
Contributor

tomjoht commented Mar 3, 2017

If you don't want to use the complete tree diagrams there, you could use forward slashes instead.

@DirtyF DirtyF added the pending-feedback We are waiting for more info. label Mar 25, 2017
@DirtyF DirtyF assigned DirtyF and unassigned parkr Mar 25, 2017
@parkr
Copy link
Member

parkr commented Mar 31, 2017

@DirtyF Where are you with this? Need more info?

@rlue
Copy link
Contributor Author

rlue commented Mar 31, 2017

Sorry to ghost you guys, I'm happy to make the changes @tomjohnson1492 requested, and will get to work on it this weekend.

@jekyllbot jekyllbot removed the pending-feedback We are waiting for more info. label Mar 31, 2017
@rlue
Copy link
Contributor Author

rlue commented Apr 3, 2017

Okay folks, I've made the changes requested.

@tomjohnson1492:

  • In the interest of consistency, I abandoned the apidocs example Collection altogether in favor of _my_collection/some_subdir/some_doc.md (which appears in the section immediately preceding Permalinks)

  • Thanks for pointing out the misplaced modifier. Hopefully the new version is to your liking?

  • Not sure exactly what was wrong with the tree diagrams in my last commit; I simply copied the format as it was before my edits. I suspect you either meant that root directories for the tree diagrams were absent; or that in the first column, vertical lines descended down from the top and didn't connect to anything underneath.

    To address the first point, I added root directories to each tree diagram. To address the second, I added ellipses to the bottom of some tree diagrams to indicate that there were irrelevant portions omitted.

Let me know if there's anything else I should do!


```yaml
collections:
my_collection:
categories:
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to replace my_collection with categorieshere?

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

Some minor changes and I think we're good!

output: true
permalink: /awesome/:path/:title.:output_ext
permalink: /category/:name
Copy link
Member

Choose a reason for hiding this comment

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

same question as above 🤔


* **Default**
Same as `permalink: /:collection/:path`.
```
Copy link
Member

@DirtyF DirtyF Apr 3, 2017

Choose a reason for hiding this comment

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

Please insert carriage returns to create a paragraph before each code sample

@rlue
Copy link
Contributor Author

rlue commented Apr 4, 2017

Good looking out!

@DirtyF
Copy link
Member

DirtyF commented Apr 4, 2017

@rlue Thanks for making Jekyll's documentation better 📗

@jekyllbot: merge +docs

@jekyllbot jekyllbot merged commit 94e6b65 into jekyll:master Apr 4, 2017
jekyllbot added a commit that referenced this pull request Apr 4, 2017
@rlue rlue deleted the patch-1 branch April 5, 2017 00:31
@jekyll jekyll locked and limited conversation to collaborators Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants