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

Change "docs/home" to "docs" #1257

Merged
merged 9 commits into from
May 7, 2020
Merged

Change "docs/home" to "docs" #1257

merged 9 commits into from
May 7, 2020

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented May 7, 2020

Address #1073 partially
With some minor changes that allow a blank string slug in sidebar.json, this can be done without making the docs homepage a special exemption.
The existing redirect from /docs to /docs/home also had to be reversed.

- Change sidebar.json to point Home to the new url and keep the same label.
- Change DocsPage model to generate home at new url.
- Swap /doc -> /doc/home redirect to the other way around.
- Change link component to allow empty strings that aren't undefined.
@shcheklein shcheklein temporarily deployed to dvc-landing-docs-home-t-zyvsg0 May 7, 2020 04:20 Inactive
@shcheklein

This comment has been minimized.

@shcheklein
Copy link
Member

and one minor thing I noticed - nav link (and it other places) ends with / for DOC

redirects-list.json Outdated Show resolved Hide resolved
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 but please check the code base for instances of doc/home. content\blog\2020-04-30-gsod-ideas-2020.md and src\gatsby\models\docs\onCreateMarkdownContentNode.js have it, at least.

@rogermparent rogermparent temporarily deployed to dvc-landing-docs-home-t-zyvsg0 May 7, 2020 16:47 Inactive
"^/doc/?$ /doc/home 307",
"^/(?:documentation)/?$ /doc",
"^/(?:documentation)/(.*)?/?$ /doc/$1",
"^/doc/home/?$ /doc 307",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"^/doc/home/?$ /doc 307",

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I still see this one in the code?

@rogermparent rogermparent temporarily deployed to dvc-landing-docs-home-t-zyvsg0 May 7, 2020 16:56 Inactive
@rogermparent
Copy link
Contributor Author

check the code base for instances of doc/home. content\blog\2020-04-30-gsod-ideas-2020.md and src\gatsby\models\docs\onCreateMarkdownContentNode.js have it, at least.

Part of this branch was removing doc/home from the docs model node creator, and it turns out after a ripgrep that that blog post was the only other instance! Thanks for pointing that out.

@rogermparent rogermparent temporarily deployed to dvc-landing-docs-home-t-zyvsg0 May 7, 2020 17:32 Inactive
@rogermparent rogermparent marked this pull request as draft May 7, 2020 17:37
@rogermparent rogermparent temporarily deployed to dvc-landing-docs-home-t-zyvsg0 May 7, 2020 17:44 Inactive
@rogermparent rogermparent marked this pull request as ready for review May 7, 2020 17:46
@rogermparent rogermparent linked an issue May 7, 2020 that may be closed by this pull request
5 tasks
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.

@shcheklein
Copy link
Member

Looks good to me, all concerns are fixed as far as I can tell.

@shcheklein shcheklein merged commit bce4d6a into master May 7, 2020
@jorgeorpinel
Copy link
Contributor

Why does the link on the top menu end with slash / ? I mean, where in the code is that? I don't see it in the source code changes.

@jorgeorpinel
Copy link
Contributor

It's weird because it only happens whe you load the site. However if you load any area outside of docs, after clicking on docs the slash goes away once you're in the docs. Not really a problem... but funny

@shcheklein
Copy link
Member

@rogermparent yes, this is not good (mean prefetch does not work and most likely we hit a redirect). I actually checked this when this branch was deployed, so not sure wha't going on here. Could you please create a ticket to investigate and fix this?

@rogermparent
Copy link
Contributor Author

Got it, I'm not sure what's up but I'll get started investigating it.

@rogermparent
Copy link
Contributor Author

Looks like some leftover Sidebar link behavior was the culprit (even the top nav bar still sources from it). I've got a quick fix at #1265 now, but it's worth further decoupling the sidebar from all the links outside of itself in the future.

@shcheklein shcheklein deleted the docs-home-to-docs branch February 28, 2021 19:36
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.

3 participants