Skip to content

Fix #21 and #28#37

Open
maiertech wants to merge 8 commits intototocaster:masterfrom
maiertech:issue-28
Open

Fix #21 and #28#37
maiertech wants to merge 8 commits intototocaster:masterfrom
maiertech:issue-28

Conversation

@maiertech
Copy link

@maiertech maiertech commented Sep 6, 2016

This PR fixes #28 and presumably #21:

  • Does not use setter to write back metadata to avoid it being cloned. This seems best practice in other plugins, e.g. metalsmith-collections.
  • I cleaned up index.js and used variable names that make reading and understanding the code easier.
  • Same for the tests. I added more tests. Main difference is that I do not test against generated and layout-processed files from the fixtures any more. This is not needed because I can test against files and verify anything I want to verify regarding the tag pages.
  • There is a breaking change: the tags for pages has now the same structure as the global tags. This is to ensure that I can re-use a layout partial to display all tags or only page tags without having to modify how to access tag and urlSafe. This might conflict with PR Add a tagsCollection object holding a display & url safe tag label #34.
  • I removed the skipMetadata option. It looked to me that it did not save much processing because the plugin still needs to create the global tags data structure.
  • I updated the README and tried to explain what the plugin does under the hood. It's not perfect, but better than before.

Copy link

@woodyrew woodyrew left a comment

Choose a reason for hiding this comment

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

Only use lodash.sortby rather than the whole package.

@@ -1,210 +1,143 @@

var _ = require('lodash');

Choose a reason for hiding this comment

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

Rather than including the whole of lodash, it would be good to include the packages needed.
e.g.

var sortBy = require('lodash.sortby');

and amend line 87 to match the variable name.

},
"dependencies": {
"slug": "^0.9.1"
"lodash": "^4.15.0",

Choose a reason for hiding this comment

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

Don't forget to change the dependency too:

"lodash.sortby": "^4.7.0"

@woodyrew
Copy link

@mdotasia Can you rebase to resolve the conflicts?

@jarodtaylor
Copy link
Collaborator

@mdotasia if you can rebase and resolve the conflicts, we'll get this PR merged in.

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.

overridding path variable in pagination.files

3 participants