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

Adds a "Guides" section to document #102

Merged
merged 6 commits into from
Jul 27, 2018
Merged

Conversation

amsaha
Copy link
Contributor

@amsaha amsaha commented Jul 24, 2018

Fixes issue #97

@amsaha
Copy link
Contributor Author

amsaha commented Jul 24, 2018

/hold

@jlewi
Copy link
Contributor

jlewi commented Jul 24, 2018

This is great; thank you.

/lgtm
/approve

Any reason for the hold? Just remove it if you think its good to go.

@amsaha
Copy link
Contributor Author

amsaha commented Jul 25, 2018

/hold cancel
@jlewi not sure why there is a conflict in "content/docs/started/pytorch.md".

@amsaha
Copy link
Contributor Author

amsaha commented Jul 25, 2018

@jlewi rebasing it fixed it :-) So just neet /lgtm label.

@amsaha
Copy link
Contributor Author

amsaha commented Jul 26, 2018

@jlewi need lgtm on this one.

@@ -0,0 +1,36 @@
+++
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, thanks for doing this! I have just one suggestion: Change the file name to use a hyphen instead of an underscore:
usage-reporting.md

Using a hyphen rather than an underscore in URLs is common practice. I see we use hyphens for all the file names in the getting-started section, and it'd be good to standardise.
https://github.com/kubeflow/website/tree/master/content/docs/started

Now that you're removing the old "user_guide.md", this is a good time to start the standardisation. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point.

@jlewi
Copy link
Contributor

jlewi commented Jul 26, 2018

This looks great.

One more comment can we sort the Guides Alphabetically in the Guides Navigation section? I think that will be make them easier to find. I don't know if there's a way to do that automatically or if we have to manually adjust the order.

@amsaha
Copy link
Contributor Author

amsaha commented Jul 26, 2018

@jlewi @sarahmaddox Making it alphabetical puts the "advanced" stuff first. Currently, I have put the "Requirements" first followed by "Ksonnet" which seems a reasonable order.

I don't think there is an easy way to order alphabetically. We will have to set the weights (hugo property) of the files properly.

@amsaha
Copy link
Contributor Author

amsaha commented Jul 27, 2018

@jlewi can you take a look at this? This has been pending for sometime now.

@jlewi
Copy link
Contributor

jlewi commented Jul 27, 2018

Sorry; was busy with next
/lgtm
/approve

/hold
This is a significant improvement so if you want to go ahead and submit it just remove the hold.

Your point about alphabetical not making sense is well taken.

The problem with the current layout is that I think a common mode is people will want to quickly find information about a particular component "e.g. seldon". In that case lack of Alphabetical ordering makes it really hard to find the information you want.

What if we had a subsection components and within that we alphabetized things

e.g.

Guides
Requirements
Components
jupyter notebooks
ksonnet
pytorch
seldon
tensorflow serving
tensorflow training
Troubleshooting
Upgrading Kubeflow
Usage Reporting

So within the components section we alphabetize but we order the other sections according to user flow

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@amsaha
Copy link
Contributor Author

amsaha commented Jul 27, 2018

@jlewi I agree. Will send another patch later in the day :-)

@amsaha
Copy link
Contributor Author

amsaha commented Jul 27, 2018

/hold cancel

@jlewi I played around with a second level inside "Guides" and it is not trivial to do. I need to play around for sometime to do it. I suggest we handle it via a separate issue (issue #105)

@amsaha
Copy link
Contributor Author

amsaha commented Jul 27, 2018

/hold cancel

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

Successfully merging this pull request may close these issues.

None yet

4 participants