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

Fix #21 and #28 #37

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fix #21 and #28 #37

wants to merge 8 commits into from

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