search

docpad / docpad-plugin-paged

Adds support to DocPad for rendering a document into multiple pages.
Other
28 stars 11 forks source link

Add split, prefix and index settings #22

Closed StormPooper closed 9 years ago

StormPooper commented 9 years ago

So @ervwalter created pull request #12 back in 2013 to add the ability to alter the URL structure to have URLs such as /archive/page/2 instead of the default /archive.2. Unfortunately, it didn't pass CI and there were no additional tests. Since I really wanted this functionality, I decided to recreate his changes on the latest version of the master branch with a couple of changes.

The three options and their defaults:

Each of the options above can be combined, and work both with and without the cleanurls plugin (and is compatible with their static redirection stuff). One of the reasons this pull request has so many files is that I have included tests for the majority of the various combinations. I've also updated the readme with details of each option and examples of both the default and changed, along with an example of combining them.

While I agree with @balupton that we could probably set the defaults to turning this on, I didn't want to make this a breaking change, though once it's in and people are happy with it, maybe it can be followed up with new defaults. Having said that, we would need to think about the possibility of deprecating the old URL structure - maybe some automatic redirection, for example.

Let me know if you have any questions or want me to change anything (I'm not 100% happy with the name split, for example, so if someone thinks of a better name then I'll update it with that).

balupton commented 9 years ago

I'm down for completing getting rid of the . scheme in favour of the new slash scheme, using the slash scheme as the new default. From there, we could just do document.addUrl('the url with the dots') and then let the cleanurl plugin take care of the redirects.

What are your thoughts?

StormPooper commented 9 years ago

I'm all for making it the default (would you want any of the other defaults changing? I'd personally prefer upping the index to 1 too).

One issue I can see with using addUrl is that it doesn't do a 301 redirect when running the site dynamically, which means Google would be indexing two copies of the same page on different URLs (I'm no SEO expert, but we've had similar concerns in my workplace recently, so it's on the brain).

Having a canonical link in the HEAD with the document URL would stop this (and probably a good idea anyway), but I'd personally prefer a 301 redirect (or at least have it configurable). I'm still fairly new to DocPad though, so I might be missing an obvious thing here.

What do you think, @balupton?

StormPooper commented 9 years ago

Just looking through the main repo and noticed pull request #905, which will fix the secondary URL redirection issue (though it hasn't been pulled in yet). I'll take a look at changing the defaults with the assumption that the #905 will be pulled in soon..

zenorocha commented 9 years ago

+1000 for this feature

StormPooper commented 9 years ago

OK, updated with two changes. The defaults now split the URL and use 1-based indexing, plus it generate the old URL structure as a secondary URLs by default, meaning URLs become:

There is one exception to the secondary URL generation. If you set the URL structure to match the old format but change the index to non-zero-based, I've had to disable the secondary URL generation, as it causes clashes with the real URLs and just gets confusing.

balupton commented 9 years ago

There is one exception to the secondary URL generation. If you set the URL structure to match the old format but change the index to non-zero-based, I've had to disable the secondary URL generation, as it causes clashes with the real URLs and just gets confusing.

Was the index configurable before? As this could be an issue if people upgrade and then the URLs don't redirect properly - which is important, as it is fine for us to change the functionality to clean it up and be smarter, but we have to ensure the old way of doing it is correctly redirected to the correct pages.

StormPooper commented 9 years ago

The index was not configurable before, but the secondary URL generates URLs based on the old zero-based index. The only time there will be a clash is if they manually configure the URL structure to match the old format, but with a different index - any other format will be compatible, from my testing. I've made a note of this in the readme too.

balupton commented 9 years ago

I wonder if we should even let people configure their url structure... The less configuration (the things we have to support in the future) the better.

... however that can be a follow up change however as it is non-essential at this point in time.

StormPooper commented 9 years ago

Personally, I'll be adding a prefix to the url to match my old WordPress url structure, so I'd argue that it should be configurable, though I see where you're coming from.

Maybe we mandate split: true and compatibility: true and only allow prefix and index to be changed. This would guarantee backwards compatibility but still offer flexibility. What do you think? I'd be happy to do that this evening, it'd only take a minute.

balupton commented 9 years ago

Hold off for now. I'll review in the next 48 hours. :-)

StormPooper commented 9 years ago

Hey @balupton, have you had a chance to review yet?

balupton commented 9 years ago

@StormPooper not yet, working on a new bugfix release of DocPad, then will do this. Sorry for the wait!

StormPooper commented 9 years ago

Hey @balupton, any word on when this can be merged in?

balupton commented 9 years ago

@StormPooper what's your npm handle? I'm not going to get time to review this due to some life things happening - but I trust your skills, so happy for you to take over as maintainer for this plugin. If something goes awry, we can always fix it :-)

StormPooper commented 9 years ago

@balupton sure, no problem, happy to help out. npm username is stormpooper.

Let me know if you need any maintainers on other plugins or DocPad, I'd be glad to help out there too.

balupton commented 9 years ago

@StormPooper you're now on the extras team, with push and publish access to all our plugins :-) :dancer:

StormPooper commented 9 years ago

Awesome, thanks @balupton :tada: