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

create site.pages with globs #186

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

karthik2804
Copy link
Contributor

@karthik2804 karthik2804 commented Dec 4, 2023

Allows to add optional frontmatter to templates to allow for reading only files specified by globs instead of all the files in the content folder.

---
read_pages_glob = "events/*.md"
---

Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
@itowlson
Copy link
Contributor

itowlson commented Dec 4, 2023

The authoring experience here relies on the assumption that each template is interested in only one glob. Consider a page with both an "upcoming events" and a "featured articles" section. Is the expectation that such a page would get broken down into three templates, one for each "area of interest" section and one for the overall page? Is that a reasonable way for an author/designer to approach such a page? Is that how we've traditionally done things?

@karthik2804
Copy link
Contributor Author

karthik2804 commented Dec 4, 2023

Only the Top template controls what is passed down to its children, we could possibly switch to globwalk and support specifying multiple globs. thoughts @itowlson?

or even something like where the read_pages_glob becomes Vec<String> without switching libraries

read_pages_glob = ["events/*.md",  "changelog/*.md"]

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

I can't really comment on functionality, but it looks like this is pretty much existing code with a different file picker strapped on the front, so I'm sure that's fine - my comments are style only.

src/content.rs Outdated Show resolved Hide resolved
src/content.rs Outdated Show resolved Hide resolved
src/content.rs Show resolved Hide resolved
@itowlson
Copy link
Contributor

itowlson commented Dec 4, 2023

Sorry missed your reply to the authoring/scope question. That sounds a bit unlovely - naively, it would be nice for each loop to only have to inspect the things in its glob - but if that's how we have to do it then that's how we have to do it - I'll take your advice on what's practical.

@karthik2804
Copy link
Contributor Author

Unfortunately the way things are laid out as is we can't make the inspecting thing per loop without it breaking. I think the array of strings is the easiest thing to do in a non breaking manner.

Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

I'm afraid I still have significant stylistic concerns, but I appreciate we need this quickly and I trust you on the functionality, so I'm okay with revisiting those in a follow-up PR if that works better for you. Thanks for the quick turnaround on this!

src/content.rs Outdated Show resolved Hide resolved
src/content.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
src/template.rs Show resolved Hide resolved
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Thank you. The new functions really help.

src/content.rs Outdated Show resolved Hide resolved
src/content.rs Outdated Show resolved Hide resolved
src/template.rs Outdated Show resolved Hide resolved
Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
@karthik2804 karthik2804 merged commit 66431f1 into fermyon:main Dec 5, 2023
2 checks passed
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