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

get-started: initial refactoring #1051

Merged
merged 16 commits into from
Mar 20, 2020
Merged

get-started: initial refactoring #1051

merged 16 commits into from
Mar 20, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Mar 15, 2020

@shcheklein shcheklein temporarily deployed to dvc-landing-refactor-ge-nouj86 March 15, 2020 19:59 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 15, 2020 23:06 Inactive
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Mar 15, 2020

@shcheklein so far this only implements #747 (creates https://dvc-landing-refactor-ge-nouj86.herokuapp.com/doc) but probably a little hacky – see getItemByPath in src/utils/sidebar.js and SidebarMenu.timeout in src/components/Documentation/SidebarMenu/index.js. Can you guys look at it and lmk what a better way to implement this could be? Cc @fabiosantoscode

Also if this should be a PR of it's own I can separate it from the remaining changes (Get Started related).

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 15, 2020 23:17 Inactive
@shcheklein
Copy link
Member

Looks good to me! We should spend a bit of time to think about the landing page content (check other projects). Also, see this discussion that is relevant to your changes in the sidebar.json - #1025 (comment) ... cc @iAdramelk - if we can merge this one before Gatsby migration we might just disallow having any entry w/o index page? But to be honest I would still do redirects if file is missing, and we should be doing this on the middleware level automatically (not relying on sidebar.json).

@shcheklein

This comment has been minimized.

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Mar 16, 2020

  • We should spend a bit of time to think about the landing page content (check other projects)

Working on that ⏳

we need to introduce some special "Home" entry to the sidebar?

I don't think so, we have the DOC link in the top menu. But if you prefer that then sure, that wouldn't require the js code changes I did.

if we can merge this one before Gatsby migration

If you want to merge this PR soon, please approve it and I'll limit it to this first step for now.

@shcheklein

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 16, 2020 00:59 Inactive
@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 16, 2020 01:51 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 16, 2020 03:24 Inactive
@jorgeorpinel

This comment has been minimized.

@shcheklein

This comment has been minimized.

@shcheklein

This comment has been minimized.

@shcheklein
Copy link
Member

shcheklein commented Mar 16, 2020

@shcheklein

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@iterative iterative deleted a comment from shcheklein Mar 16, 2020
@jorgeorpinel jorgeorpinel changed the title get-started: big refactoring get-started: initial refactoring Mar 17, 2020
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 18, 2020 19:49 Inactive
@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 18, 2020 21:02 Inactive
@jorgeorpinel jorgeorpinel requested a review from shcheklein March 18, 2020 21:02
@jorgeorpinel

This comment has been minimized.

redirects-list.json Outdated Show resolved Hide resolved
"^/doc/understanding-dvc/?$ /doc/understanding-dvc/collaboration-issues 307",
"^/doc/changelog/?$ /doc/changelog/0.18 307",
Copy link
Member

Choose a reason for hiding this comment

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

I really want to get rid of changelog, make it an external link ... again a special case like a Home button ... we spend time on it again and again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was an external link already before? I didn't introduce this in the Gatsby migration, just moved the order of some redirects.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 19, 2020 07:37 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 19, 2020 07:40 Inactive
@jorgeorpinel jorgeorpinel mentioned this pull request Mar 19, 2020
5 tasks
@jorgeorpinel jorgeorpinel requested a review from shcheklein March 19, 2020 07:47
@jorgeorpinel
Copy link
Contributor Author

Let's try to address the remaining refactoring in #1074 (or further PRs)? Any problems specific to these nav order changes I can still fix here, of course.

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.

Looks good to me! Make sure that all pages we move we make redirects from their previous locations. Feel free to merge!

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-refactor-ge-nouj86 March 20, 2020 21:18 Inactive
@jorgeorpinel jorgeorpinel merged commit 859763c into master Mar 20, 2020
@jorgeorpinel jorgeorpinel deleted the refactor/get-started branch March 21, 2020 03:01
@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) p1-important Active priorities to deal within next sprints C: start Content of /doc/start labels Mar 16, 2023
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 p1-important Active priorities to deal within next sprints
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: move /get-started index to root and /install above /get-started
2 participants