-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
600 Refactor supported versions #606
Conversation
@mrjones-plip fresh perspective would be useful on this PR. Some of my questions:
|
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! one, one char fix needed though
content/en/core/releases/_index.md
Outdated
aliases: | ||
- core/overview/supported-software |
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.
YAY! so happy to see this - great minds think alike ;)
However, leading slash needed to work correctly (then we'll match indentation while we're at it):
aliases: | |
- core/overview/supported-software | |
aliases: | |
- /core/overview/supported-software |
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 eye on the slash! The leading spacing was off in my commit, and will match the spacing between the dash and path to that of the other uses of aliases
.
oop! I missed a few questions there - sorry! tl;dr - no changes needed I think. Musings below.
Yes, good a location as any. BTW - nice addition to the hosting requirements!
yes to this too - it even is future proof with
Now that you have me looking at it - I could see an argument to move the "Requirements" section (dependencies & client devices) as a peer of "Releases" maybe? But it's such a small amount of content - it's fine where it is. I do with the "Release Notes" could be in columns though to save on vertical scrolling. No love though without some new templates (ironically, no two column on their home page?). |
Reordered to match other lists on page, as per PR feedback
Thanks for the feedback @mrjones-plip! I have gone ahead and reordered the dependencies as you suggested. |
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!
For #600