-
Notifications
You must be signed in to change notification settings - Fork 1
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
Reviewed Changes #7
Conversation
DarikshaAnsari
commented
Jul 24, 2024
- Removed the in-toto website link from the navbar.
- Added the Learn More section back to the navbar.
- Added three more pages: News, Security Audit, and FAQ under Learn More section.
- Changed a "Software Supply Chain Protection" card button to link to system overview.
- Changed "Open, Extensible Standard" card icon to fa-book.
✅ Deploy Preview for in-toto-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
@lukpueh and @DarikshaAnsari:
- There should not be a top-nav entry named "Learn more", instead I propose the following.
- Learn-more/faq should be under
/docs/faq
- Learn-more/security-audit-23 should be a blog post under a Blog section. Blog should be what appears in the top nav
- Learn-more/news can appear as a News entry in the top-nav
Also see inline comments
@chalin, I have made all the changes you suggested. PTAL !!! |
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.
Good progress. See inline comments.
Just to clarify, in my review on slack I didn't mean to re-add the "Learn more" top-nav, but rather to migrate the contents from under the old "Learn more". The structure you suggest for this sounds reasonable. :) Speaking of my review. @DarikshaAnsari Did you plan to address my other comments as part of this PR? It's okay if not. Just wanted to make sure they are not lost. I'll paste them again here:
|
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.
Sure, I have these points in my mind and will open a separate PR for these issues after this PR merges. |
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.
Overall, good changes. See inline comments.
One main issue remains regarding the specs page.
What you should have in this PR is the following:
data/specs.yaml
- same as the current sitelayouts/shortcodes/spec-table.md
- shortcode that generates the specs table in Markdown; it will be very similar to the current site'slayouts/shortcodes/specs.html
shortcode, but will use Makrdown instead of HTML in the shortcode. If you can't figure this out, embed the hardcoded table for now and I'll show you how we can do this in a followup PR.content/en/docs/specs.md
- a page that simply references the shortcode using{{% spec-table %}}
.
Yes, I have already removed data/specs.yaml, layouts/shortcodes/spec-table.md and use inline markdown table in spec-table.md |
Clearly there's a misunderstanding. As I mentioned in my review comment, the bulleted list describes what this PR should contain. In the quoted comment of yours above you are confirming that you removed "data/specs.yaml", but it needs to be re-introduced into this PR -- that is, it should not have been removed in the first place. Do you understand what I meant in #7 (review)? If it isn't clear then we can have a video call to clarify. Let me know. |
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 few more comments, see inline.
It is clear now. Sorry for the misunderstanding. |
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.
@lukpueh Can I merge this PR and start working on other issue, so that there is no conflict. |