-
Notifications
You must be signed in to change notification settings - Fork 229
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
nonspec: New navigation #1275
nonspec: New navigation #1275
Conversation
Signed-off-by: Arnaud J Le Hors <[email protected]>
Signed-off-by: Arnaud J Le Hors <[email protected]>
Signed-off-by: Arnaud J Le Hors <[email protected]>
Signed-off-by: Arnaud J Le Hors <[email protected]>
✅ Deploy Preview for slsa ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Arnaud J Le Hors <[email protected]>
Signed-off-by: Arnaud J Le Hors <[email protected]>
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.
Looks good, a lot nicer than our existing navigation.
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, thanks @lehors ! Only one small change.
docs/_data/nav/main.yml
Outdated
url: /spec/v1.0/onepage | ||
skip_next_prev: true # don't show as a next/prev link | ||
|
||
- title: SLSA Specification 1.1 |
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 noticed that SLSA 1.0 will appear above the more recent 1.1. I think it may be helpful to have the most recent release appear at the top of the spec section of the navigation menu (WD can still be at the bottom).
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.
Really? I was actually wondering whether we should even include 1.1 because I don't think we're going to move this spec any further as it stands. Indeed, based on all the discussions we had, I think it should rather be 1.0.1.
The one advantage I see to having it anyway, and possibly first as you suggest, is to show activity since 1.0 and dispel the idea that SLSA is dead.
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.
Right, my main reasoning was really that whichever version is the most recent (whether it's 1.0.1 or 1.1), if it's on top, viewers of the website don't need to scroll down to find it.
EDIT: fwiw, this isn't a blocker for me. If you want to test out how the website works with your current updates, and maybe consider reordering spec versions later, we can also do that later.
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.
No worries, this is easy enough to change any time. I made the switch.
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.
So, 1.1 still isn't ready for prime time, e.g. there are still 'TODOs' in there: https://deploy-preview-1275--slsa.netlify.app/spec/v1.1/threats#:~:text=TODO%3A%20Update%20the%20ordering%20to%20match%20the%20diagram.%20We%E2%80%99re%20currently%20in%20the%20middle%20of%20refactoring%2C%20with%20a%20jumble%20of%20new%20and%20old.
So I can see a case for not including it, or it needs to be made clear in the sidebar that it's still draft.
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 don't think the big banner that shows up at the top makes that clear?
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.
Honestly it's pretty easy to miss. On this page my eyes go straight to the "SLSA Specification" heading and completely ignore the banner.
Option: add a '(current)' label on the nav bar item for 1.0?
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.
Alright then!... I didn't think that banner could be missed but ok. I just added the word 'Draft' to the navigation bar. The name is getting long and folds onto 2 lines but this might be seen as a feature. :-)
Signed-off-by: Arnaud J Le Hors <[email protected]>
Thanks for doing this Arnaud. At the spec meeting earlier this week what you'd shown had separate top-level pages for the new tracks. I don't see that here. Have you decided against that approach or will that come with a subsequent PR? |
I haven't. I'm just doing this in several steps because I think they are different decision points. Even if we decide not to separate the tracks I think there is value in this new navigation bar. I'll do more in follow-up PRs. |
Signed-off-by: Arnaud J Le Hors <[email protected]>
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 again for this @lehors ! I agree with you that the 1.1 menu label wrapping around 2 lines isn't ideal, but we can hopefully address it soon by switching to RC or releasing it :)
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 Arnaud!
Strictly speaking this PR only needs 2 approvals for merging but given the impact I'd like to have a couple more. |
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 looks good, though I played with it via the deploy preview rather than every entry in yml file(s). Thanks for working on this!
Ok, I think we've got enough people in favor of this change to merge. |
Here is my first take on the new navigation approach.
This PR doesn't make any changes to how the specification is organized with regard to the tracks, etc. It is meant to be merely a UI modification, essentially changing the navigation bar so that the various versions of the specification are directly visible and accessible rather than dependent on using the version selector which only appears once you go into the specification.
I didn't include the older versions: 0.1 and 1.0-rc1 and rc2. All of these are however still there and can be accessed directly via their respective URLs or from any page linking to them such as past blog posts. Let me know if you think we should add 0.1 to the navigation bar. I'm also not sure there is value in having the 1.1 RC given that I think it's a dead-end (if anything I think this should be 1.0.1).
In the process of making this change I found a few bugs that I was able to fix.
Any feedback or suggestions welcome but please keep in mind that Jekyll is a static page generator so anything that requires dynamic processing (either server-side or client-side) is out of scope. These would require PHP and/or Javascript. I've more or less managed to get my head around Jekyll and its template programming language Liquid but I'm not up for the added complexity any of this would imply.
PR #1268 will have a small impact that I'll handle once it is merged.