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: misc content & guide updates #2359

Merged
merged 29 commits into from
May 12, 2021

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented Apr 6, 2021

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Please split into 2 or hopefully more PRs, it's too big 🙂 Thanks!

I didn't review the Get Started changes yet, just the concepts pages.

@casperdcl
Copy link
Contributor Author

casperdcl commented Apr 7, 2021

Please split into 2 or hopefully more PRs, it's too big 🙂 Thanks!

I thought it would be a bit overwhelming. This PR is only about 20% of the start of the basic documentation copyediting I'm planning before moving on to high-level reorganising and then major new content.

Good job I stopped myself from including the remaining 80% of the "basic start" in this PR :)

You may find the "Viewed" checkbox useful when reviewing multi-file PRs.

image

@casperdcl casperdcl self-assigned this Apr 7, 2021
@casperdcl casperdcl added the A: docs Area: user documentation (gatsby-theme-iterative) label Apr 7, 2021
@iesahin
Copy link
Contributor

iesahin commented Apr 7, 2021

You like your dashes :)

I read the PR and it's a nice touch. Thank you very much.

BTW, the dataset/code in the GS section will be updated. We'll probably use MNIST dataset for a new example-get-started. Most of the content will need another revision then. You can keep this in mind when revising/updating.

@casperdcl
Copy link
Contributor Author

You like your dashes :)

I actually don't like them but was simply aiming for consistency first (dashes were already used in more places than hyphens). The "correct" way really is double hyphen -- which is auto-rendered as a dash but that's for a separate PR :)

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Apr 8, 2021

thought it would be a bit overwhelming. This PR is only about 20% of the start of the basic documentation

Trust me I know all about how quickly docs PRs can grow. It's a constant struggle to find the right balance. In this case I perceived the Get Started changes to have repeating patterns so that group of changes I think could've been a PR. Then there were the basic concepts which could've been another. Then all the other misc. changes seemed OK for a PR (which would be merged at this point).

We don't have a well defined rule yet, but let's work on it together. The one thing I can tell you is we prefer the problem of having many PRs to having big PRs!

You may find the "Viewed" checkbox useful when reviewing multi-file PRs.

The technical problem with large docs PRs comes when the review goes on for many rounds with dozens of comments in each one. GH starts hiding some content (resolved or not) which makes navigating the PR page quite difficult.

double hyphen -- which is auto-rendered as a dash

😮 I didn't realize this, yeah def better! I actually have a special keyboard shortcut just for m-dash 😆

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Review for content/docs/start/data-and-model-access.md. Some comments can probably be generalized to the remaining files, so I'll review the others when this one has been addressed 👇 Thanks!

content/docs/start/data-and-model-access.md Show resolved Hide resolved
content/docs/start/data-and-model-access.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-access.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-access.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-access.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-access.md Outdated Show resolved Hide resolved
casperdcl added a commit to casperdcl/dvc.org that referenced this pull request Apr 9, 2021
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Review of content/docs/start/data-and-model-versioning.md 🙌 Thanks!

content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
casperdcl added a commit to casperdcl/dvc.org that referenced this pull request Apr 9, 2021
@jorgeorpinel

This comment has been minimized.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Please make a separate issue/PR if needed to introduce the "storage location" term throughout docs, as we have an established way to refer to "remote storage" and "DVC remote(s)" ATM. For now:

content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
content/docs/start/data-and-model-versioning.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-docs-misc-fixes-seg0k4 April 14, 2021 06:46 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

OK! Here's the review for https://dvc-org-docs-misc-fixes-seg0k4.herokuapp.com/doc/start/data-pipelines. As you can see each page has a long review, which is why I still think we should split the PR. It's getting unmanageable at this point.

Please: Each GS page should be a PR, including one to address this review (so we can wrap up all the previous comments and merge this one), then one for Metrics/Plots/Params, and one for Experiments. Keep in mind you're editing a very important section with lots of strategic words, passages, etc. Even sentence lengths were designed. Each page took weeks to finalize. So we have to be extra careful.

And again, please try to minimize style/tone/subjective changes in the remaining GS pages based on previous reviews.

Thanks @casperdcl, we're getting there and many of the edits here are going to improve the readability a lot!

content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
content/docs/start/data-pipelines.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-docs-misc-fixes-seg0k4 April 14, 2021 07:45 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-docs-misc-fixes-seg0k4 April 14, 2021 07:45 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-docs-misc-fixes-oa4brd May 10, 2021 19:02 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Seems like @dberenbaum is busy. These are copy edits anyway so here's the review of the one last file pending. Thanks!

content/docs/start/experiments.md Outdated Show resolved Hide resolved
content/docs/start/experiments.md Outdated Show resolved Hide resolved
content/docs/start/experiments.md Outdated Show resolved Hide resolved
content/docs/start/experiments.md Outdated Show resolved Hide resolved
content/docs/start/experiments.md Outdated Show resolved Hide resolved
content/docs/start/experiments.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-docs-misc-fixes-oa4brd May 10, 2021 19:15 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-docs-misc-fixes-oa4brd May 10, 2021 19:15 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-docs-misc-fixes-oa4brd May 10, 2021 19:26 Inactive
Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org>
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-docs-misc-fixes-oa4brd May 10, 2021 21:24 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-docs-misc-fixes-oa4brd May 10, 2021 21:24 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-docs-misc-fixes-oa4brd May 12, 2021 06:35 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jorgeorpinel jorgeorpinel merged commit 94479dd into iterative:master May 12, 2021
@jorgeorpinel
Copy link
Contributor

Kaboom.

@casperdcl casperdcl deleted the docs-misc-fixes branch May 12, 2021 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: docs Area: user documentation (gatsby-theme-iterative)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: wrong use of diff highlighting + indentation problem
5 participants