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

start: cleanup nav #3941

Merged
merged 3 commits into from
Sep 15, 2022
Merged

start: cleanup nav #3941

merged 3 commits into from
Sep 15, 2022

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Sep 14, 2022

In preparation for #3943

Ideally approve/merge along with #3942.

@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) C: start Content of /doc/start labels Sep 14, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-start-cleanup-d780abpa September 14, 2022 02:43 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-start-cleanup-d780abpa September 14, 2022 02:45 Inactive
Comment on lines -47 to +45
{
"label": "Data and Model Access",
"slug": "access"
},
{
"label": "Data Pipelines",
"slug": "pipelines"
},
"data-and-model-access",
"data-pipelines",
Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Sep 14, 2022

Choose a reason for hiding this comment

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

  • Significantly simplifies this section of sidebar.json.
  • Updates 2 URL paths so they match page titles (good for SEO, best practice).

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing source from things might break the "Edit on GitHub" links unless we update the sidebar logic to better accept them. Might not, but we need to make sure to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm thanks for letting me know @rogermparent. I did not know that (probably something we can improve on in the engine...) I'll reinstate the sources for now ⏳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unless we update the sidebar logic to better accept them

iterative/gatsby-theme-iterative#77

Might not, but we need to make sure to check

P.s. I did check and the button links were broken indeed (in the review app).

Copy link
Contributor

Choose a reason for hiding this comment

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

The button on the review app were broken because the link generated based on master/main tree. I checked by changing the path to current branch tree and it's working.

The logic is intact to link the file source using a simple string slug. And, the source field is also getting populated accordingly.
Screen Shot 2022-09-14 at 15 32 52

Copy link
Contributor

@rogermparent rogermparent Sep 14, 2022

Choose a reason for hiding this comment

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

That would be the case for some of them, but the result of the logic on some nested items, for example docs/start, render the link as https://github.com/iterative/dvc.org/blob/main/content/docs/start.md where the file is actually at https://github.com/iterative/dvc.org/blob/main/content/docs/start/index.md. We can likely just make a change that assumes source-less files are at index.md and that'd be all we need to support 90+% of source-less cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I already rolled this back for now. Feel free to take it to iterative/gatsby-theme-iterative#77 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I checked on the rolled-back version of the preview app.

Yes, we do need to define the source for items with index.md.

Comment on lines -72 to 75
### Experiments Trail
### Experimentation

- [**Experiments**](/doc/start/experiments) enable exploration, iteration, and
comparison across many ML experiments. Track your experiments with automatic
Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Sep 14, 2022

Choose a reason for hiding this comment

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

Should we call this trail something else? Maybe "Machine Learning Experimentation" ?

Comment on lines -41 to +47
"^/doc/start/data-versioning$ /doc/start/data-and-model-versioning",
"^/doc/start/data-access$ /doc/start/data-and-model-access",
"^/doc/start/data-and-model-versioning(/.*)?$ /doc/start/data-management$1 302",
"^/doc/start/data-and-model-access(/.*)?$ /doc/start/data-management/access$1 302",
"^/doc/start/data-pipelines(/.*)?$ /doc/start/data-management/pipelines$1 302",
"^/doc/start/metrics-parameters-plots(/.*)?$ /doc/start/data-management/metrics-parameters-plots$1 302",
"^/doc/start/sharing-data-and-model-files$ /doc/start/data-and-model-versioning#storing-and-sharing 302",
"^/doc/start/data-versioning$ /doc/start/data-management",
"^/doc/start/data-and-model-versioning(/.*)?$ /doc/start/data-management 302",
"^/doc/start/sharing-data-and-model-files$ /doc/start/data-management#storing-and-sharing 302",
"^/doc/start/data-access$ /doc/start/data-management/data-and-model-access",
"^/doc/start/data-and-model-access(/.*)?$ /doc/start/data-management/data-and-model-access 302",
"^/doc/start/data-pipelines(/.*)?$ /doc/start/data-management/data-pipelines 302",
"^/doc/start/metrics-parameters-plots(/.*)?$ /doc/start/data-management/metrics-parameters-plots 302",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and simplified redirects as well (using final locations for previous 301 ones, plus fortunately many were 302s so we can change those any time).

jorgeorpinel added a commit that referenced this pull request Sep 14, 2022
@github-actions
Copy link
Contributor

Link Check Report

There were no links to check!

@jorgeorpinel
Copy link
Contributor Author

Also, I made a separate PR to update all the links to keep this PR lean. PTAL: #3942

@shcheklein
Copy link
Member

Preview link https://dvc-org-start-cleanup-d780abpa.herokuapp.com/doc (not working here for some reason)

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Let's not mix many things at once (e.g. data pipelines - we actually go with Pipeline now, right?)

do we need to remove a few index files here?

@jorgeorpinel jorgeorpinel mentioned this pull request Sep 14, 2022
1 task
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Sep 14, 2022

Preview link https://dvc-org-start-cleanup-d780abpa.herokuapp.com/doc (not working here for some reason

Something with Heroku's SSL certs proabbly. You can chose to "visit this unsafe site":

image

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Sep 14, 2022

Let's not mix many things at once

Sorry @shcheklein, not sure what you mean. This is a nav cleanup with 2 URL changes. Should I make a PR for each one?

e.g. data pipelines - we actually go with Pipeline now

In the User Guide yes, but in Get Started the title is already Data Pipelines, this PR just fixed the URL so it reflects that title. We can change it, but that would be a separate change IMO. Let's discuss including that in #3943 ?

@jorgeorpinel
Copy link
Contributor Author

We can change it, but that would be a separate change IMO.

Or I can return the file to pipelines.md and update the title to just Pipelines as well. Either route works for me as long as they're consistent. Regardless, it's a change (perhaps larger).

Lmk please!

@shcheklein
Copy link
Member

@jorgeorpinel okay, I see I got confused since it also cleanups a lot of stuff along the way ... if this doesn't break any then LGTM ;)

@shcheklein shcheklein temporarily deployed to dvc-org-start-cleanup-d780abpa September 15, 2022 07:34 Inactive
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Sep 15, 2022

I did include other closely related goals (to my mind), 3 in total. It's def. not easy to understand just from the diff which is why I left comments (1, 2, 3). Glad it's clearer now. Thanks!

@jorgeorpinel jorgeorpinel merged commit e8e4949 into main Sep 15, 2022
@jorgeorpinel jorgeorpinel deleted the start/cleanup branch September 15, 2022 07:39
jorgeorpinel added a commit that referenced this pull request Sep 15, 2022
* start: cleanup nav
in prep for #3678

* start: update links from all docs
on top of #3941
jorgeorpinel added a commit that referenced this pull request Sep 15, 2022
* start: cleanup nav
in prep for #3678

* start: update links from all docs
on top of #3941

* start: md ref links (index)
jorgeorpinel added a commit that referenced this pull request Sep 21, 2022
* start: cleanup nav
in prep for #3678

* start: update links from all docs
on top of #3941

* start: md ref links (index)

* start: make data versioning page

* start: write new intro DM page and
move the rest to the new DV page (old DM content)
per #3955 (comment)
jorgeorpinel added a commit that referenced this pull request Sep 21, 2022
* start: cleanup nav
in prep for #3678

* start: update links from all docs
on top of #3941

* start: md ref links (index)

* start: make data versioning page

* start: write new intro DM page and
move the rest to the new DV page (old DM content)
per #3955 (comment)

* start: update links from DM index to new DV page
@jorgeorpinel jorgeorpinel added the type: enhancement Something is not clear, small updates, improvement suggestions label Nov 3, 2022
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) C: start Content of /doc/start type: enhancement Something is not clear, small updates, improvement suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants