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

docs(readme): show only one level in table of contents #136

Merged
merged 1 commit into from
Apr 25, 2020
Merged

docs(readme): show only one level in table of contents #136

merged 1 commit into from
Apr 25, 2020

Conversation

noelmcloughlin
Copy link
Member

This PR makes the table of contents less verbose (depth: 1).

Table of Contents

General notes
Contributing to this repo
Available states
Testing

The default depth (two) is too verbose for users.

The list of available states is not impacted by this update.

@noelmcloughlin
Copy link
Member Author

OpenSuse Travis CI job failed. bundler: failed to load command: kitchen (/home/travis/build/saltstack-formulas/template-formula/vendor/bundle/ruby/2.4.0/bin/kitchen)

@noelmcloughlin noelmcloughlin requested a review from myii June 16, 2019 22:50
@myii
Copy link
Member

myii commented Jun 25, 2019

@noelmcloughlin I can see advantages to both ways, so I'm not convinced on proceeding with the merge. The main reason being is that the current method has been in use for a long time (i.e. before all of these recent developments) so using this method would be inconsistent until it eventually propagated through all of the repos out there. The other thing is that we really should have the proper documentation solution in place before this propagation can complete, so that will introduce even more inconsistencies in the interim. Essentially, it may end up being a change for no real gain. However, I'm not going to get in the way if this is preferred by others as well.

@noelmcloughlin
Copy link
Member Author

noelmcloughlin commented Jun 25, 2019

The repetition of "Available States" from usage of both "..contents:: table of contents" and "contents:: local" is unnecessary and removing the repetition feels more user friendly. An alternative option is to remove .. contents:: table of content from the README. However since documentation is WIP I can close this PR. wdyt

@myii
Copy link
Member

myii commented Jun 25, 2019

@noelmcloughlin No need to close it, this is a valid idea. It's just horses for courses. Personally, I've got very used to seeing everything at the top, even since dealing with SaltStack Formulas. So I'd suppose that there are a few others like me, who wouldn't like to have to scroll down to "find" the states' list. Yes, there's some duplication but it doesn't cause any harm from an RST perspective. Still, some may suggest we should have one or the other, not both. And then there are some who prefer both...!

@noelmcloughlin
Copy link
Member Author

It's hard to RTFM on some formula with many states. Lots of scrolling with mouse.

@myii myii changed the base branch from master to develop July 24, 2019 21:41
Copy link
Contributor

@baby-gnu baby-gnu left a comment

Choose a reason for hiding this comment

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

I agree that we shouldn't have 2 listings. It should be either in TOC or locally under Available states.

I approve this PR since I find it more readable for new users:

  1. Synopsis
  2. Table of content to have a quick look and an easy jump
  3. Explanation about the formula, mostly for new user I think ;-)

@myii
Copy link
Member

myii commented Sep 19, 2019

@daks @n-rodriguez Can I pull you both in for this PR and number #169? You've both done a number of semantic-release conversions. Your opinions would be very useful, since this will lead to changing the structure across all of the formulas.

Personally, I'm OK either way. My only concern is consistency; whatever we agree on should be used across the org.

@aboe76 aboe76 requested a review from daks September 20, 2019 20:08
@n-rodriguez
Copy link
Member

I approve this PR since I find it more readable for new users

#metoo

Copy link
Member

@myii myii left a comment

Choose a reason for hiding this comment

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

While reviewing the lvm-formula PR, this requires adding the following under some/most of the main headings:

.. contents::
    :local:

So that will be needed here as well, when working in conjunction with #169.

@myii
Copy link
Member

myii commented Sep 24, 2019

@noelmcloughlin Actual, since both ideas are getting approvals, why not fix #169 in this PR as well? That makes it easier to work out where the local contents are going to be needed.

@myii myii changed the base branch from develop to master October 13, 2019 21:16
@noelmcloughlin
Copy link
Member Author

I'll handle #169 in another PR because this is now an "unknown branch". /shrug
Thanks, everyone for reviewing. Appreciated.

@noelmcloughlin noelmcloughlin merged commit 44bc362 into saltstack-formulas:master Apr 25, 2020
@saltstack-formulas-travis

🎉 This PR is included in version 4.0.9 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants