-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[blog] moved blog files to posts sub dir #2720
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
Conversation
Signed-off-by: Victoria Nduka <ndukavictoria7@gmail.com>
|
This breaks the build. I think there are some other pieces missing if we move all these files. For example, we are looking at the blog-posts/ directory in this helpers file - https://github.com/prometheus/docs/blob/main/src/blog-helpers.ts#L5 |
point blogPostsDir to blog-posts/posts to restore build Signed-off-by: Victoria Nduka <122698422+nwanduka@users.noreply.github.com>
|
@sysadmind you're right. thanks for pointing it out. |
sysadmind
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@juliusv you had the feedback on the previous PR, do you want to weigh in on this one? |
bwplotka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for challenging this, but what are the rationales? 🙈
The directory is called blog-posts. Now it will be blog-posts/posts. Is there a reason to duplicate posts naming here? Additionally blog-posts will not have any more files (other than README). Are we not making it harder to contribute blog posts with this change? 🤔 Maybe I miss context, do you mind @nwanduka to explain some long term plan for this change in this PR?
|
I guess it all goes back to the discussion I started at #2718 (comment) - I don't mind that much whether the blog post instructions file is in the root or in a blog-specific subdir, I just found it cleaner to not have it in the same directory as all the blog post files themselves, since that requires an explicit code-level exception and also makes the new file harder to find by a human if it's in the same dir as all the blog post content files. But yeah, maybe just calling the first dir |
| import path from "path"; | ||
| import matter from "gray-matter"; | ||
|
|
||
| const blogPostsDir = "blog-posts"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are doing this, let's also remove the filter from line 38 🙈
Also maybe blog/posts might be clearer (as @juliusv mentioned)
|
Ah got it, I didn't see the previous discussion on my fix. Then let's remove the Sounds like we have two options here then (happy with both) A) Change dirs, remove coding filter.
B) Do nothing (: Thanks for helping @nwanduka! |
|
Yeah, the previously introduced special-case filtering should be removed now. Then let's do option A I would say. |
Signed-off-by: Victoria Nduka <ndukavictoria7@gmail.com>
0c7f04e to
6173726
Compare
|
Thanks a lot everyone (@sysadmind @bwplotka @juliusv) for reviewing and leaving your feedback. I appreciate it. I've made the necessary changes (that's option A) and I think this PR is ready for a final review. |
bwplotka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
In this PR, I: