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

Implemented getting output dir from eleventyConfig & manual override #14

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

Conversation

Denperidge
Copy link

Hi! Small addition, but it decreases the setup work needed.

@NJAldwin
Copy link
Owner

NJAldwin commented Dec 5, 2024

Hi, thanks for the PR! That does seem like a nice addition.

I will have some more time to review this in depth a bit later. I have a couple of small questions:

  1. For backwards compatibility, can we preserve the original name of outputDir?
  2. I seem to remember having some issue with this hence being why it's not included. Perhaps that's not an issue anymore, but would it be possible to add a test of the behaviors just to be safe?

Thank you!

@Denperidge
Copy link
Author

Denperidge commented Dec 5, 2024

  1. Makes perfect sense!
  2. As in, testing that the parameter for sure overrides the eleventy.dir.output usage? Will already whip something like that up, but let me know if you meant something different

Edit: is it possible to test eleventy config behaviours in the current testing setup? because genFavicons doesn't handle the default-dir behaviour, but index.js instead

One of the scenarios outputs on _site, which it shouldn't
Good news:
- eleventy-test wasn't malfunctioning; the bug it gave was an actual bug in my code addition here
- NJAldwin was fully correct with point 2.
NJAldwin#14 (comment)
eleventyConfig.dir does not keep CLI in check
However, eleventyConfig.directories does
@Denperidge
Copy link
Author

Denperidge commented Dec 7, 2024

Hi hi! Excuse the many WIP commits (I can squash them together if wanted), but I've added tests for it by using eleventy-test (which I have just made, admittedly) and even figured out what your previous past issues might've been from!

If this test sounds okay for you:

  • I will make sure it also works on Eleventy 2 by expanding the tests
  • I can optionally add tests in there for plugin-in-Eleventy testing!

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.

2 participants