FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: auto generate sidebar by sy-records · Pull Request #130 · docsifyjs/docsify-cli · GitHub

/ docsify-cli Public

feat: auto generate sidebar - #130

Merged
sy-records merged 4 commits into
masterfrom
gen
Jan 22, 2021
ConversationCommits (4)ChecksFiles changed
Merged

feat: auto generate sidebar#130
sy-records merged 4 commits into
masterfrom
gen

Conversation

sy-records commented Jan 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

I have tested it under mac, can anyone test it in other environments?

feat: docsifyjs/docsify#1290

fix: docsifyjs/docsify#1323
fix: docsifyjs/docsify#1444

sy-records requested review from a team, anikethsaha and jamesgeorge007 January 15, 2021 08:07
jamesgeorge007 reviewed Jan 16, 2021
View reviewed changes

jamesgeorge007 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Let's rename it to generate. Can you write tests?

Koooooo-7 reviewed Jan 16, 2021
View reviewed changes
Comment thread lib/commands/generate.js
Koooooo-7 approved these changes Jan 16, 2021
View reviewed changes

Koooooo-7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality


LGTM.

Koooooo-7 commented Jan 16, 2021

Copy link
Copy Markdown
Member

FYI, I run the npm run test .\e2e\cli.test.js , it seems failed due to the i18n stuff. is there something wrong ?

jamesgeorge007 requested changes Jan 18, 2021
View reviewed changes

jamesgeorge007 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looking good, left a couple of suggestions.

Comment thread README.md Outdated
Comment thread lib/commands/generate.js Outdated

if (!exists(sidebarPath)) {
genSidebar(cwdPath, sidebarPath)
console.log(chalk.green(`Generate sidebar file '${sidebar}' success.`))

jamesgeorge007 Jan 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
console.log(chalk.green(`Generate sidebar file '${sidebar}' success.`))
console.log(chalk.green(`Successfully generated the sidebar file '${sidebar}'.`))

jamesgeorge007 Jan 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It is better to have a logger utility. There are a couple of places that require an update; feel free to address it in a future PR.

- console.error(chalk.red(msg))
+ logger.error(msg)
// logger.js
const error = (msg) => console.error(chalk.red(msg));

sy-records Jan 19, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

After the merge, I will submit a PR to amend this

Comment thread lib/commands/generate.js Outdated
Comment thread lib/commands/generate.js Outdated
Comment thread lib/commands/generate.js Outdated
Comment thread e2e/commands/generate.test.js
Apply suggestions from code review
916479a 
Co-authored-by: James George <jamesgeorge998001@gmail.com>
sy-records merged commit e83bfcb into master Jan 22, 2021
sy-records deleted the gen branch January 22, 2021 05:24
This was referenced Mar 15, 2021
Merged
Merged
Closed
Anilople mentioned this pull request Nov 4, 2021
Closed
trusktr mentioned this pull request Jan 26, 2022
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Koooooo-7 Koooooo-7 approved these changes
jamesgeorge007 jamesgeorge007 approved these changes
anikethsaha anikethsaha approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Does not automatically generate a left-hand directory Proposal: Allow :include in sidebar

4 participants

Back | FazBrowse Home | New Git URL