Skip to content
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

Merged
merged 3 commits into from
Jan 14, 2022
Merged

600 Refactor supported versions #606

merged 3 commits into from
Jan 14, 2022

Conversation

abbyad
Copy link
Contributor

@abbyad abbyad commented Jan 11, 2022

For #600

@abbyad abbyad requested a review from mrjones-plip January 11, 2022 22:32
@abbyad
Copy link
Contributor Author

abbyad commented Jan 11, 2022

@mrjones-plip fresh perspective would be useful on this PR.

Some of my questions:

  • Does the dependency and requirements language resonate here?
  • Is the dependency table still accurate and relevant?
  • Should the device requirements be moved out elsewhere, perhaps as requirements under CHT Apps?

@abbyad abbyad linked an issue Jan 11, 2022 that may be closed by this pull request
@abbyad abbyad changed the title Refactor supported versions 600 Refactor supported versions Jan 11, 2022
Copy link
Contributor

@mrjones-plip mrjones-plip left a 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

Comment on lines 8 to 9
aliases:
- core/overview/supported-software
Copy link
Contributor

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):

Suggested change
aliases:
- core/overview/supported-software
aliases:
- /core/overview/supported-software

Copy link
Contributor Author

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.

@mrjones-plip
Copy link
Contributor

oop! I missed a few questions there - sorry! tl;dr - no changes needed I think. Musings below.

Does the dependency and requirements language resonate here?

Yes, good a location as any. BTW - nice addition to the hosting requirements!

Is the dependency table still accurate and relevant?

yes to this too - it even is future proof with 4.0 in there 🚀 . The only minor tweak, now that I'm looking at it closely, is that all other tables/lists on this page are descending sorted and this is ascending sorted. Maybe worth a re-order? Very minor.

Should the device requirements be moved out elsewhere, perhaps as requirements under CHT Apps?

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
@abbyad abbyad marked this pull request as ready for review January 13, 2022 20:26
@abbyad
Copy link
Contributor Author

abbyad commented Jan 13, 2022

Thanks for the feedback @mrjones-plip! I have gone ahead and reordered the dependencies as you suggested.

@abbyad abbyad requested a review from mrjones-plip January 13, 2022 20:27
Copy link
Contributor

@mrjones-plip mrjones-plip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@abbyad abbyad merged commit 3488924 into master Jan 14, 2022
@abbyad abbyad deleted the 600-supported-versions branch January 14, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move 'Supported Versions' content
2 participants