Fixes #305 & #286 #375
Fixes #305 & #286 #375
Conversation
|
package.json
Outdated
| @@ -26,6 +26,8 @@ | |||
| "axios": "^0.19.0", | |||
| "babel-plugin-transform-define": "^1.3.1", | |||
| "color": "^3.0.0", | |||
| "eslint": "^5.16.0", | |||
| "eslint-plugin-react": "^7.13.0", | |||
There was a problem hiding this comment.
are they used somewhere now? what about lodash - we removed it below, should we remove it from the package.json?
There was a problem hiding this comment.
we installed it for ourselves, we want to set up a beautiful code for your code style
There was a problem hiding this comment.
it should be done as a separate discussion, separate PR. let's remove it from this PR for now.
pages/doc.js
Outdated
| import { scroller, animateScroll } from 'react-scroll' | ||
| import 'core-js/fn/array/find-index' | ||
| // styles | ||
| import styled from 'styled-components' | ||
| import { media } from '../src/styles' | ||
| // json | ||
| import sidebar from '../src/Documentation/sidebar' | ||
| import SidebarMenuHelper from '../src/Documentation/SidebarMenu/SidebarMenuHelper' | ||
| import { PATH_SEPARATOR } from '../src/Documentation/SidebarMenu/SidebarMenuHelper' |
There was a problem hiding this comment.
looks a bit too much, it's always '/' - it's easier to use just plain '/' everywhere.
pages/doc.js
Outdated
| } | ||
| let path = window.location.pathname.split(PATH_SEPARATOR) | ||
| let length = path.length | ||
| let { file, indexes } = SidebarMenuHelper.getFileFromUrl(path) |
There was a problem hiding this comment.
can we make this function actually return what is needed for the loadFile below? why should I care here about some indexes I don't know anything about. It looks like I care about section, subsection, and file. Let's return them?
There was a problem hiding this comment.
loadFile works with files of different levels in sidebar.json, so this function needs all the information.
| return `/doc/${compact([sectionSlug, fileSlug]).join('/')}` | ||
| getLinkHref = (section, subsection = null, file = null) => { | ||
| let sect = sidebar[section] | ||
| let removeExtFunc = filename => |
There was a problem hiding this comment.
let's just rename this function in the SideHelper in the first place
There was a problem hiding this comment.
so, is it fixed? forgot to push the changes?
There was a problem hiding this comment.
Sorry, were not careful, renamed to SidebarHelper, now fix
| </Menu> | ||
| ) | ||
| } | ||
| renderSection = (section, file, index, fileIndex) => { |
There was a problem hiding this comment.
please, add spaces between functions
pages/doc.js
Outdated
| let subsect = sect.files[subsection] | ||
| let subfolder = subsect && subsect.folder | ||
| let folderpath = file.folder || subfolder || sect.folder | ||
| let filepath = file.indexFile || file.files || file |
There was a problem hiding this comment.
file.files[0]?
all these logic ^^ is still very complicated
can we make it always accept file as a file, not as a section or something else?
There was a problem hiding this comment.
we can if we make the sidebar.json structure the same for any type of element
pages/doc.js
Outdated
| const subsectionSlug = | ||
| (subsection && removeExtFunc(sect.files[subsection].indexFile)) || | ||
| sect.files[subsection] | ||
| const fileSlug = removeExtFunc(file) || (file && file.files[0]) |
There was a problem hiding this comment.
three lines above extremely hard to understand. For example why file is not actually file sometimes? is there a really strong reason here?
The same for subsection.
There was a problem hiding this comment.
When parsing, the next element in the sidebar.json structure may turn out to be a string, this is because each level may contain a different structure and a different set of fields.
pages/doc.js
Outdated
| this.setCurrentPath( | ||
| section, | ||
| subsection, | ||
| file.indexFile ? file.indexFile : file |
There was a problem hiding this comment.
same ... is file or not? what is the reason to pass different structures around?
pages/doc.js
Outdated
| let subsect = sect.files[subsection] | ||
| let subfolder = subsect && subsect.folder | ||
| let folderpath = file.folder || subfolder || sect.folder | ||
| let filepath = file.indexFile || file.files || file |
There was a problem hiding this comment.
same here, previous lines are compilcated.
should it be actuallu files[0]?
There was a problem hiding this comment.
so, is file.files a bug? should it be file.files[0]?
There was a problem hiding this comment.
This function is used if the section (User Guide for example) does not contain its own root file, in which case the application suspects that the required file should be searched for in the attached file, and the latter may also be an object or a string.
| array.forEach(elem => { | ||
| let path = SidebarMenuHelper.getFullPath(folder, elem) | ||
| if (path === currentFile) { | ||
| flag = true |
There was a problem hiding this comment.
during the passage of the array, in the end we need only the last value
| .split(PATH_SEPARATOR) | ||
| .filter(_ => _) | ||
|
|
||
| static combineToPath = subPaths => [].concat(subPaths).join(PATH_SEPARATOR) |
There was a problem hiding this comment.
just curious - why is [].concat(subPaths) needed? subPath is always non empty array, right?
There was a problem hiding this comment.
It will make function safe, exception won’t be thrown even if subPaths is null or undefined. May be it’s too much, agreed.
| static findFileByName = (item, find) => { | ||
| let file = null | ||
| if ( | ||
| SidebarMenuHelper.removeExtensionFromFileName(item.indexFile || item) === |
There was a problem hiding this comment.
install pre-commit hook and/or run prettier, it's > 80 symbols
~70% modifications complete
Fixes #305
Fixes #286