-
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
Added New pages #11
Added New pages #11
Conversation
DarikshaAnsari
commented
Aug 5, 2024
•
edited by chalin
Loading
edited by chalin
- Fixes Misc comments #8
✅ 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.
A first round of comments.
Please run npm run fix:format
and commit the changes.
content/en/docs/demo.md
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.
@DarikshaAnsari - I believe that you copied this from the README of the demo repo. We don't want the same information in two places, so either the README will need to be shortened, and a link added to this page (once it becomes a part of the official site), or we can find a way later to fetch the page a build time.
I'll let @lukpueh comment on his preference 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.
I've created the following to track this issue, since @lukpueh and I agree that we do not want replicated content:
b0b4d73
to
d20c971
Compare
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, once the paths to images are fixed in Adopters.
cc:- @lukpueh |
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.
Nice progress! Though it does not fully address #8, or at least not as requested. Did you disagree with my comments? Or was I just unclear. :)
- I like that the buttons and links on the landing page navigate to subpages inside docs. Maybe we can make this more consistent. That is, we map all buttons/links on the landing page to subpages . E.g. we could make "demo" and "friends" subpages in docs, to which "Try the demo" and "Explore integrations" point to. Those pages can then contain a short description of demo and friends repos and pointers to those repos. I think that might be a less disruptive experience, compared to when the user is directed to external pages right away on the landing page.
By short description I meant a paragraph or so and not the complete README from the demo and friends repos. Replicating all of this content on the website means that we have to either maintain it in two places, or remove it in its original place. Both of which does not seem desirable.
- Why are there two search bars, one in the right top corner and one in the left navigation column? (no need to fix this now, I'm just curious)
This does not seem resolved.
- Regarding content I am not sure if we should follow the recommendation from the cncf assessment and just re-publish the docs from python-in-toto as Getting started page. The documentation should represent the whole in-toto project, but the python-in-toto cli is only one of multiple in-toto implementations.
Same as above with demo and friends content.
Hey @lukpueh, you're right, but I believe the friends section looks nice on the website rather than residing in the README of any GitHub repository. Additionally, for the demo, it's beneficial for all the contributors and integrators of the project to have the demo on one website instead of having to jump to another link placed on GitHub. I think you get my point. I've taken reference from many CNCF documentation websites that do exactly the same, including OpenTelemetry,KCL and many more. |
If you don't want to fix this now, but close #8, please submit another issue so we can fix this separately. |
I think this is the layout of google docsy. |
Okay, I'm still not happy of maintaining the exact same content in two places, but I won't insist. Let's have other in-toto project maintainers weigh in, when we merge this upstream. |
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.
You can fix the get started link in a separate PR.
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 going along with @lukpueh's approval. We can address the remaining issues via:
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.
Please rebase and ensure that all tests pass. Other than the one suggestion (see the inline comment), this LGTM under the assumption that you'll be following up with further changes.
content/en/_index.md
Outdated
<a class="btn btn-lg btn-primary me-3" href="https://github.com/in-toto/demo">Try the demo</a> | ||
<a class="btn btn-lg btn-primary" href="https://github.com/in-toto/friends">Explore integrations</a> | ||
<a class="btn btn-lg btn-primary me-3" href="docs/demo">Try the demo</a> | ||
<a class="btn btn-lg btn-primary" href="ecosystem/">Explore integrations</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.
A link to the "Ecosystem" is in the top nav. We don't need a CTA button for this IMHO.
<a class="btn btn-lg btn-primary" href="ecosystem/">Explore integrations</a> |
396f3b2
to
cecef90
Compare
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.
Run npm run fix:format
and commit changes; otherwise, this LGTM
I have used
|
Right. You'll need to merge #27 first, then rebase this PR, resolve conflicts, re-run fix:format, and commit the results :). |