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

Allow the asset dir to be overridden #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dannystaple
Copy link

With this change, the front matter for a page can specify an alternate asset dir. Useful on legacy sites with plenty fo images in another folder structure.

With this change, the front matter for a page can specify an alternate asset dir. Useful on legacy sites with plenty fo images in another folder structure.
@oturpe
Copy link
Owner

oturpe commented Jun 6, 2021

Thank you for your contribution, it is great to see that this generator is useful for someone! Yes, I think it should be possible to use different asset path layouts than the default. In order to leave the door open for even more options, in casse somebody needs them, I would like to split this into two configuration options:

  • assets_base - changing this only changes the base folder name assets to whatever is set
  • assets_structure - enum with possible values collection_doc (the existing, default behavior) or base (the new option)

This way, just defining the assets folder does not change the expected folder structure, and changing the folder structure does not change the base folder name. How does this sound to you?

Also, README should be updated to document the new feature.

@oturpe
Copy link
Owner

oturpe commented Jun 6, 2021

Another point that came to my mind: If this new option is used, then using the cover.png as the default cover name does not make much sense any more. Do you think it would make sense to not apply any default in your case, just not include a thumbnail if it is not set explicitly in document front matter?

@dannystaple
Copy link
Author

I will update the readme - good idea.
So to deal with my use case - I would set assets_base to my url, and assets_structure to base?
I can build that. And I guess this would still be in front matter, although potentially with the following scope resolution (painters algorithm - last one wins):

  • Default "assets"
  • Config
  • Front matter

It sounds like the default cover.png behaviour was useful in your use case. Perhaps some config option to say what that default behaviour should be would be helpful? Ie an implied mode of cover.png relative to the assets base, or explicit (only checking when explicitly set)?

@oturpe
Copy link
Owner

oturpe commented Jun 7, 2021

I will update the readme - good idea.

Thank you!

So to deal with my use case - I would set assets_base to my url, and assets_structure to base?
I can build that.

Yes, that is what I meant.

And I guess this would still be in front matter, although potentially with the following scope resolution (painters algorithm - last one wins):

* Default "assets"

* Config

* Front matter

Hmm, I now see that I misunderstood your pull request. I thought the value was being fetched from config. So I was only talking about the first two points. But the combination of what you submitted and I promised (i.e. the above three layer method) is actually the best. I like it.

It sounds like the default cover.png behaviour was useful in your use case. Perhaps some config option to say what that default behaviour should be would be helpful? Ie an implied mode of cover.png relative to the assets base, or explicit (only checking when explicitly set)?

Yes, in my case that was useful. I can see how it would not be in other cases. One idea that comes to mind would be to add a new entry to the thumbnail config, default. If set, that is the default to use. If not set, then docs that do not define their cover image do not have one. That is backwards incompatible, but easy to fix on upgrade and I do not think this plugin has a large amount of users, so it is not a big deal.

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