-
-
Notifications
You must be signed in to change notification settings - Fork 76
Feature/forum topics fetch #584
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
base: master
Are you sure you want to change the base?
Conversation
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 for this PR, looks very nice!
Some first comments to consider. I will do a more thorough review later.
- title: Forum Browser | ||
url: /community-forum-browser.html | ||
output: web |
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.
A more fitting place would be in the docs_sidebar.yaml
, under Fundamentals
, between Roadmap
and Previous versions
.
A better title would be FAQ
and initialized with the faq
tag in all categories.
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 have made the improvements as suggested. Let me know if it works.
assets/data/forum/topics.json
Outdated
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.
This data should not be directly in the repository. It should either:
a) be always loaded on demand when the page is loading, or
b) be stored in Git LFS and updated periodically with a GitHub Action.
Option (a) would be the easier to implement, option (b) could potentially be faster. Let's start with option (a).
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.
The check old API us
pre-commit hook is also failing, because it is picking up keywords in the JSON file. Removing the JSON file from the repository (and loading it every time it is needed) would fix that.
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.
@BytePaul this file still needs to be removed from this branch (and the system needs to fetch the data on demand).
See #576 for an example of how to implement a button to fetch the data. Rendered on https://precice.org/fundamentals-roadmap.html
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.
This originates from #582. Please rebase on master
and remove the unrelated files.
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.
@BytePaul FYI, I will wait for the cleanup before I continue with the rest of my review.
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.
@BytePaul the changes to this file still need to be removed from this branch.
194424e
to
a980cb5
Compare
- Relocated sidebar entry from community_sidebar.yaml to docs_sidebar.yaml - Placed FAQ under Fundamentals, between Roadmap and Previous versions - Renamed 'Forum Browser' to 'FAQ' - Added faq tag to enable categorization
47ed4af
to
c4ed1da
Compare
This PR adds a new "Forum Browser" page under the Community section.