Fix #305 #527

iAdramelk · 2019-08-07T16:37:08Z

This PR should fix #305. It contains following changes.

New format for sidebar.json. It removes distinction between sections and files and allows any number of nesting levels.

New format (full):

{
  "slug": "metrics",
  "label": "metrics",
  "source": "metrics/index.md",
  "children": []
}

slug - Required. URL part for the current link, will be also used as URL prefix for all children of the current category. Example: metrics -> /doc/metrics. For the child addof metrics -> /doc/metrics/add.

label - name of the menu item. If not set, it will be generated from slug using startCase from lodash. Generating from slug example: get-started -> Get Started.

source -> name of the corresponding .md file in the filesystem. If not set, it will be generated from slug. Generating from slug example: metrics -> metrics.md. For legacy reasons you can explicitly set this field to false, in this case after user select this item, its first child will be shown instead.

children - optional array of child items.

You can also use short syntax. metrics is the same as { "slug": "metrics" }.

This config is parsed using new src/Documentation/SidebarMenu/helper.js file. It will normalize data from "sidebar.json" and generate all necessary field. It's output structure is:

{
  label: "Add Files or Directories",
  path: "/doc/get-started/add-files",
  source: "/static/docs/get-started/add-files.md",
  prev: "/doc/get-started/configure",
  next: "/doc/get-started/share-data",
  children: []
}

This way we don't need to calculate them in react component anymore.

This file also exports two new helper functions:

getItemByPath - recursively search for items in sidebar, that matches given path. It also resolve some edge cases (like source = false, and /doc/ route).
getParentsListFromPath - returns all parents paths from current path. Example: /doc/commands-reference/destroy will return ['/doc/commands-reference', '/doc/commands-reference/destroy']. Used to make nescessary menu items active.

pages/doc.js, src/Documentation/Markdown/Markdown.js and src/Documentation/SidebarMenu/SidebarMenu.js are updated accordingly, all business logic related to parsing sidebar.json are removed from them and replaced with data from new normalized menu items.

Difference from the current version:

Prev/Next are generated for all pages, not only for pages inside the same section.
Right now each nesting level in menu has different left margin. In this version all nested levels always add 20px.

- Changed sidebar format so it can now have any number of levels - All json transformations and normalizations moved to src/Documentation/SidebarMenu/helper.js and done on the first init - Simplified docs.js, Markdown.js and SidebarMenu.js

pages/doc.js

- Fix bug then parent was added to prev/next order after children - Fix bug the prev item didn’t have source and was redirecting back to first child - Add check for items with source = false and without children, they throw erros now

shcheklein · 2019-08-07T18:55:03Z

src/Documentation/SidebarMenu/helper.js

+const FILE_ROOT = '/static/docs/'
+const FILE_EXTENSION = '.md'
+
+let prevReference // We will save prev item reference here to use for the next and prev fields


it's definitely > 80 symbols. I would recommend to install the pre-commit hook: https://dvc.org/doc/user-guide/contributing-documentation#development-environment

and run the prettier from the command line once again to reformat what has been committed already

I think prettier ignores comments, so it probably didn't help. But I will fix comment lengths.

you might be right - it just does not know what can be done with it.

shcheklein · 2019-08-07T19:04:04Z

pages/doc.js

+    if (!item) {
+      this.setState({ pageNotFound: true, currentItem: {} })
+
+      return


it's probably better to use else instead of an explicit return

Ok, I will change it.

Fixed bug with nested intems with osurce false, it required large refactor of the normalization function.

shcheklein · 2019-08-07T22:54:00Z

src/Documentation/SidebarMenu/helper.js

+}
+
+function normalizeSidebar(data, parentPath, currentSidebar) {
+  data.forEach(item => {


ie11? not that it's very important to keep supporting it, but if it's the only that breaks it I would do some workaround

forEach works in IE starting with IE9 according to https://caniuse.com/#feat=es5 , so it should not be a problem.

shcheklein · 2019-08-07T23:03:52Z

src/Documentation/SidebarMenu/helper.js

+
+  const { label, slug, source } = item
+
+  // If prev item don't have source we need to recirsively search for it


minor: don't -> doesn't

shcheklein · 2019-08-07T23:12:17Z

src/Documentation/SidebarMenu/helper.js

+// Global cache vars used in normalization
+
+let prevReference // Save last item here to generate the prev field
+const normalizedSidebar = [] // Current state of sidebar to search for prev


Current state of sidebar to search for - a bit misleading. It's not only a "cache" to search for prev. This is what we export then as a final result, right?

Will try to move them inside function and if I wasn't able to, will change text.

shcheklein · 2019-08-07T23:12:57Z

src/Documentation/SidebarMenu/helper.js

+  }
+}
+
+// Global cache vars used in normalization


can we pass them to functions, do we actually need a global prevReference, for example?

I have an idea how it can be done, but I'm not sure that it will make code easier to understand, but I will try today.

shcheklein · 2019-08-07T23:17:16Z

@iAdramelk https://dvc-org-pr-527.herokuapp.com/doc/understanding-dvc/what-is-dvc.md - not found page is broken + the page itself is broken?

- Fix typo in comment text - Fix error in slug name in sidebar.json - Move fetch inside else clause

iAdramelk · 2019-08-08T10:55:16Z

@shcheklein Thie problem with https://dvc-org-pr-527.herokuapp.com/doc/understanding-dvc/what-is-dvc.md is that I forgot to remove .md extension form the slug field, my bad. Fixed it and checked other pages to make sure that this is the only one.

iAdramelk · 2019-08-08T11:12:49Z

Yep, removed prevReference and current result from global scope successfully.

shcheklein · 2019-08-08T15:56:13Z

src/Documentation/SidebarMenu/SidebarMenu.js

+import { getParentsListFromPath } from './helper'
+
+function SidebarMenuItem({ children, label, path, activePaths, onNavigate }) {
+  const isActive = activePaths && activePaths.includes(path)


use lodash.includes, otherwise we are breaking compatibility

You are right, will change.

shcheklein

Amazing stuff 🎉 :)

iAdramelk added 3 commits August 7, 2019 20:09

Add nested paths

62fea87

- Changed sidebar format so it can now have any number of levels - All json transformations and normalizations moved to src/Documentation/SidebarMenu/helper.js and done on the first init - Simplified docs.js, Markdown.js and SidebarMenu.js

Fixed typo in config

3b2f1c8

Return docSearch-lvl0 class for parent dir

aef9d62

shcheklein reviewed Aug 7, 2019

View reviewed changes

pages/doc.js Show resolved Hide resolved

shcheklein temporarily deployed to dvc-org-pr-527 August 7, 2019 17:47 Inactive

Return key for ReactMarkdown to force animations

5f5de79

shcheklein temporarily deployed to dvc-org-pr-527 August 7, 2019 18:01 Inactive

Fix next/prev order

1a86a08

- Fix bug then parent was added to prev/next order after children - Fix bug the prev item didn’t have source and was redirecting back to first child - Add check for items with source = false and without children, they throw erros now

shcheklein temporarily deployed to dvc-org-pr-527 August 7, 2019 18:19 Inactive

shcheklein reviewed Aug 7, 2019

View reviewed changes

Fix for nested source = false

e125a81

Fixed bug with nested intems with osurce false, it required large refactor of the normalization function.

shcheklein temporarily deployed to dvc-org-pr-527 August 7, 2019 20:19 Inactive

shcheklein reviewed Aug 7, 2019

View reviewed changes

Fix issues form review

21e86d7

- Fix typo in comment text - Fix error in slug name in sidebar.json - Move fetch inside else clause

shcheklein temporarily deployed to dvc-org-pr-527 August 8, 2019 10:52 Inactive

Move prevReference and current result inside

5749ace

shcheklein temporarily deployed to dvc-org-pr-527 August 8, 2019 11:11 Inactive

Remove useles comment

ebcf470

shcheklein temporarily deployed to dvc-org-pr-527 August 8, 2019 11:21 Inactive

shcheklein reviewed Aug 8, 2019

View reviewed changes

Switch to loadash.includes

6dc7127

shcheklein deployed to dvc-org-pr-527 August 8, 2019 16:02 View deployment

Merge remote-tracking branch 'origin/master' into gf-fix#305_3

141dcd1

shcheklein requested a deployment to dvc-org-pr-527 August 8, 2019 16:43 Abandoned

shcheklein approved these changes Aug 8, 2019

View reviewed changes

shcheklein merged commit 765ffc5 into iterative:master Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #305 #527

Fix #305 #527

iAdramelk commented Aug 7, 2019

shcheklein Aug 7, 2019

shcheklein Aug 7, 2019

iAdramelk Aug 7, 2019 •

edited

Loading

shcheklein Aug 7, 2019

shcheklein Aug 7, 2019

iAdramelk Aug 8, 2019

shcheklein Aug 7, 2019

iAdramelk Aug 8, 2019

shcheklein Aug 7, 2019

iAdramelk Aug 8, 2019

shcheklein Aug 7, 2019

iAdramelk Aug 8, 2019

shcheklein Aug 7, 2019 •

edited

Loading

iAdramelk Aug 8, 2019

shcheklein commented Aug 7, 2019

iAdramelk commented Aug 8, 2019

iAdramelk commented Aug 8, 2019

shcheklein Aug 8, 2019

iAdramelk Aug 8, 2019

shcheklein left a comment


		const { label, slug, source } = item

		// If prev item don't have source we need to recirsively search for it

Fix #305 #527

Fix #305 #527

Conversation

iAdramelk commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iAdramelk Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein commented Aug 7, 2019

iAdramelk commented Aug 8, 2019

iAdramelk commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein left a comment

Choose a reason for hiding this comment

iAdramelk Aug 7, 2019 •

edited

Loading

shcheklein Aug 7, 2019 •

edited

Loading