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

Introduce PathSpecification #65

Merged
merged 7 commits into from
Jul 3, 2017

Conversation

GeertvanHorrik
Copy link
Contributor

This is WIP. This is the first change that migrates some properties to methods (so we don't have to expose the whole config to a nested navigation tree).

By default it falls back to full, which is the current behavior. I'll be adding unit tests as well.

@GeertvanHorrik
Copy link
Contributor Author

It auto-extracts the path from the child levels. So in case an auto-index is being generated, it will generate index.htm inside the folder it has an index to. This will also allow the end-users to navigate to a folder in any browser.

image

Even multiple levels work:

image

@GeertvanHorrik
Copy link
Contributor Author

Finally have something nice working :-)

image

@GeertvanHorrik
Copy link
Contributor Author

And of course, if you don't use this setting, everything stays as-is.

@FransBouma
Copy link
Owner

FransBouma commented Jun 26, 2017

Looks good! Does this also address this issue? #44

@GeertvanHorrik
Copy link
Contributor Author

At the moment it still generates the .htm extension, but I think this could easily be achieved (as a separate option). If you really want to get rid of the .htm, you should need to create a folder for each node and generate an index.htm in the root. I think RelativeAsFolder could be an option (so Full | Relative | RelativeAsFolder).

@GeertvanHorrik
Copy link
Contributor Author

Just pushed so you can already look at the code. In the meantime I'll wait for the example site to be updated.

@GeertvanHorrik
Copy link
Contributor Author

Hmmm, this PR doesn't seem to pickup my recent commit, which is available on the fork.... https://github.com/GeertvanHorrik/DocNet/commits/pr/path-specification

@GeertvanHorrik
Copy link
Contributor Author

Here is an example:

http://docs.catelproject.com/vnext/reference/Catel.Core/Catel/Collections/

I am now working on the RelativeAsFolder feature

@GeertvanHorrik
Copy link
Contributor Author

The RelativeAsFolder is also supported. The only downside is that the images are now 1 path above. It can be fixed automatically, but that requires that we pass in the pathspecification into the MarkdownDeep file.

@GeertvanHorrik
Copy link
Contributor Author

Written a magic workaround inside MarkdownDeep.cs. If an image cannot be resolved, it will try x times (in this time 1) levels up to see if the image can be found there.

@GeertvanHorrik
Copy link
Contributor Author

And RelativeAsFolder is also working. Maybe I'll even choose that over the Relative since it looks nice ;-)

image

@GeertvanHorrik
Copy link
Contributor Author

Fixes #44, #57 and #64

@GeertvanHorrik
Copy link
Contributor Author

Still 2 things I'd like to improve, then this one is ready for review (will comment once it hits that state).

@GeertvanHorrik
Copy link
Contributor Author

Relative paths are now result in a very clean root:

image

@GeertvanHorrik
Copy link
Contributor Author

image

@GeertvanHorrik
Copy link
Contributor Author

Ready for review!

@GeertvanHorrik
Copy link
Contributor Author

For an example of a website with the new RelativeAsFolder path specification, see http://docs.catelproject.com/vnext/ /cc @AndreyAkinshin (that's how I got to DocNet in the first place, via BenchmarkDotNet)

@GeertvanHorrik
Copy link
Contributor Author

btw once you accept this PR, I'll write the docs as well.

@FransBouma
Copy link
Owner

Thanks! I'll try to make time later this afternoon to look at the code and if everything is OK merge it :)

@FransBouma
Copy link
Owner

FransBouma commented Jun 27, 2017

There's a bug somewhere, because when I generate our docs, the Home link is empty:
was:

<li class="tocentry"><a href="index.htm">Home</a>

is:

<li class="tocentry"><a href="">Home</a>

This is a specified index for __index:

    "IncludeSource": "Shared",	
    "Pages": 
    {
		"__index": "index.md",

I haven't specified anything, just used the config files I had with my existing docs and ran the build with your code.

(edit) rest seems to be equal

@GeertvanHorrik
Copy link
Contributor Author

That's easy to fix, gimme a few minutes.

@GeertvanHorrik
Copy link
Contributor Author

Done.

@FransBouma
Copy link
Owner

FransBouma commented Jun 27, 2017

Still a small glitch. The Index.htm is needed here as otherwise clicking the link will show the folder contents

"Pages": 
{
	"__index": "index.md",
	"Welcome!": "docconventions.md",
	"What's new in v5.2": "whatsnew.md",
	"Migrating your code": "migratingcode.md",
	"LLBLGen Pro Runtime Framework features": "supportedfeatures.md",
	"Getting Started" :
	{
		"__index": "getting_started.md",
		"Quick start guides" :
		{
			"From Database to source code": 
			{
				"__index": "qsg\\dfscenario1\\index.md",

it goes wrong when an __index key has an index.md file.

Was:

<li><span class="navigationgroup"><i class="fa fa-caret-down"></i> <a href="../../qsg/dfscenario1/index.htm">From Database to source code</a></span></li>

is

<li><span class="navigationgroup"><i class="fa fa-caret-down"></i> <a href="../../qsg/dfscenario1/">From Database to source code</a></span></li>

(edit) this is needed for local browsing. For a webserver, it will likely serve up the index.htm there, but local browsing doesn't work here: it will show the folder contents, so the 'index.htm' has to be in the URL.

@GeertvanHorrik
Copy link
Contributor Author

We can:

  1. Disable it for now (it's a single extension method so the "hard" work is done)
  2. Make this a separate setting
  3. Only enable this when PathSpecification = RelativeAsFolder

I think option 3 is the best (where it makes sense).

@FransBouma
Copy link
Owner

In the case of option 3, does it then still work if I browse locally and have PathSpecification set to RelativeAsFolder? As it needs an index.htm for everything in that case (otherwise the browser will show the folder :/)

@GeertvanHorrik
Copy link
Contributor Author

RelativeAsFolder will still generate an index.htm for every .md file but in their own folder, but will not create the links as such (because, people chose that they want stuff being put into folders).

That's why you can use urls like these:

http://docs.catelproject.com/vnext/reference/Catel.Core/Catel/Argument/

(which auto-links to /vnext/reference/Catel.Core/Catel/Argument/index.htm, but it won't work locally so RelativeAsFolder is not smart if you need local viewing capabilities).

@GeertvanHorrik
Copy link
Contributor Author

The issue here is we need to decide how important it is that all path specifications can be viewed locally. The whole point of RelativeAsFolders is that you can use a folder-based navigation (with nice urls without .htm extensions). We could put it as a separate option though (RemoveIndexHtmFromLinks) or something like that.

@FransBouma
Copy link
Owner

Wasn't the whole point of PathSpecification=Relative was that the folders are relative to the parent's folder? At least that will fix an issue with clashing names, see #64. I want everything to work locally, which automatically means it will work on a server. So a feature that's added to fix an issue, like this one, has to do that by creating a site that works locally, otherwise it's besides the point. The 'nice' URLs without .htm extensions are 'nice to have' but IMHO not really important, it's first key the name clash issue is solved, which can be done by using paths relative to the parent container.

What is 'RelativeAsFolders' exactly? As I was under the assumption PathSpecification has two options: Full and Relative, and it's solely about how to construct paths to files and where to locate them

@GeertvanHorrik
Copy link
Contributor Author

Full => put all the index files in the root
Relative => put all the index files in the right folder
RelativeAsFolder => put every document in a custom folder so every document has index.htm (always)

@GeertvanHorrik
Copy link
Contributor Author

I'll remove the index.htm replacement for now, but then this doesn't fix #44

@GeertvanHorrik
Copy link
Contributor Author

Ready for another try.

@FransBouma
Copy link
Owner

Sorry, didn't have time today! Will try tomorrow.

@FransBouma
Copy link
Owner

Due to no time today due to work, I'll have a look at this next monday.

@FransBouma FransBouma merged commit d099b79 into FransBouma:master Jul 3, 2017
@FransBouma
Copy link
Owner

I don't like commented out blocks of code but I've left it there for now as it's properly commented so it's clear why it's there and why it's commented out.

@GeertvanHorrik
Copy link
Contributor Author

Thanks for the review + merge. The plan is to create 1 or 2 more PR's (the 404 + the index.htm option). Then I'll either remove the comments or fix it.

@FransBouma
Copy link
Owner

I recon you'll add a few lines to the docs as well about this?

@FransBouma
Copy link
Owner

Thanks for the review + merge. The plan is to create 1 or 2 more PR's (the 404 + the index.htm option). Then I'll either remove the comments or fix it.

Cool :)

@GeertvanHorrik
Copy link
Contributor Author

Already working on the docs!

@FransBouma
Copy link
Owner

Already working on the docs!

:) I've bumped the version to 0.16, will wait for your other PR(s) before releasing a new 'release'.

@AndreyAkinshin
Copy link

@GeertvanHorrik, the PR looks great, thank for very much for solving #44.

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.

3 participants