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

Check for collection docs and include them in generator #33

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mmenanno
Copy link

@mmenanno mmenanno commented Nov 24, 2023

This resolves #32

I grab first all the collection names, then I iterate through them pulling out their docs. These are then included in the flattened array that the existing documents method returns.

This PR still needs tests added to it but seemed to be working when I tested locally on my example repo from my issue.

@mmenanno
Copy link
Author

I just added tests and in testing realised that posts are also considered a collection so I was able to remove the site.posts.docs call in the documents method since it was redundant and resulting in posts getting pulled into documents twice.

@mmenanno
Copy link
Author

mmenanno commented Nov 24, 2023

There were some rubocop violations unrelated to this PR that I fixed:

  1. RSpec/SubjectDeclaration was mad about the let(:subject) use, instead of subject(:test_subject) (it also wasn't happy if I tried to do subject(:subject)
  2. RSpec/NamedSubject was then mad after the above that I hadn't used the named subject test_subject instead of just calling subject
  3. RSpec/SpecFilePathFormat
    was unhappy that the path was jekyll-default-layout instead of jekyll_default_layout.
  4. RSpec/ContextWording
    was mad about a few context blocks that didn't start with when

@benbalter
Copy link
Owner

@halorrr I'm supportive of this functionality, but I suspect it will be a breaking change for many sites (that currently have collections rendered without a default layout). What do you think about making this feature opt-in, or potentially opt-in per collection? Also, what is the advantage of this approach vs. defining a per-collection default layout via frontmatter defaults?

@mmenanno
Copy link
Author

mmenanno commented Dec 9, 2023

@benbalter I didn't actually know about frontmatter defaults until you mentioned them, but reading about them I think this entire plugin could be replaced by using frontmatter defaults, so I'm not sure I can give a good reason for the advantage, other than awareness of options by the user base. Your plugin was referenced in the github pages docs which is what pointed me toward it.

I tested for my own usage of this plugin for my docs site was replicated with:

defaults:
  - values:
      layout: "default"

Which did apply to collections as well.

That being said, my only pushback to this being a opt-in feature, is that I don't really see any other config options in this plugin currently, so it would be a new pattern for this plugin. Might make more sense to just bump the major version number and add to the changelog about the new behaviour.

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.

Support For Collections
2 participants