JSRocksHQ / harmonic

The next static site generator
http://harmonicjs.com/
MIT License
282 stars 26 forks source link

Fixing #73 #97

Closed jaydson closed 9 years ago

jaydson commented 9 years ago

Review needed.

//cc @UltCombo

UltCombo commented 9 years ago

Sorry man, I've been rather inactive on GitHub lately as I'm working nearly 24/7 due to a late deadline. Anyway, I'll try to review this once I get home.

jaydson commented 9 years ago

Ok, no problem @UltCombo :) @robsongajunior or @felipenmoura Please make a review on this PR.

By review i mean, run the branch locally and test if the pages are being generated well.

UltCombo commented 9 years ago

@jaydson I've just tried your branch.

I've created a page in both pages/en/foo.md and pages/pt-br/foo.md through harmonic new_page "foo", then built the site. Here are a couple issues I've found:

  1. The generated pt-br page ended up in the same path as the posts -- that is, [lang]/[year]/[month]/[filename]/index.html, is this intended? Also, I couldn't find the generated en page.
  2. The generated markup has 2 extra sets of </body></html> towards the end of the file (probably an issue with the template):

    </body>
    </html>
    
    </body>
    </html>
    </body>
    </html>
  3. It would be nice to have unit tests to cover this.

Note to self: must rebuild after git pull.

jaydson commented 9 years ago

Thanks @UltCombo ! For the item 1, i know what it is. You'll need to change the harmonic.json file:

"pages_permalink" : ":language/pages/:title",
jaydson commented 9 years ago

The item 2 should be something related to the es6rocks template.

jaydson commented 9 years ago

Item 3 :+1:

UltCombo commented 9 years ago

You'll need to change the harmonic.json file:

Perhaps it could be the new default value for pages_permalink? The posts_permalink one already contains :language.

Also, I'm a bit concerned that the logic seems to rely on having :language/ in the beginning of the permalink -- if changing it will break things, perhaps the user should not be able to change it until we support it? What I mean is, we could prepend the :language/ part implicitly and hide it from harmonic.json until we support customizing the position of the :language. Or, we should at the very least document that it is not supported to change the position of the :language segment for the time being.

Apart from this, LGTM.

UltCombo commented 9 years ago

@jaydson you could add :language/ to the pages' default permalink and merge this PR, then we can discuss the issues outlined above in a new issue. What do you think?

jaydson commented 9 years ago

Ok, i'll do it.

UltCombo commented 9 years ago

By the way, not sure if this is the best approach but instead of .split() I would replace all instances of :language with an empty string and then collapse consecutive slashes, e.g.:

options.structure =  config.pages_permalink.replace(/:language/g, '').replace(/\/{2,}/g, '/');
UltCombo commented 9 years ago

@jaydson ping. Would you want me to take a stab at this?

jaydson commented 9 years ago

Wow. This is so old i can't even remember what i've done.