Skip to content

feat: auto generate sidebar - #130

Merged
sy-records merged 4 commits into
masterfrom
gen
Jan 22, 2021
Merged

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

Conversation

@sy-records

@sy-recordssy-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

jamesgeorge007
jamesgeorge007 reviewed Jan 16, 2021
View reviewed changes

@jamesgeorge007jamesgeorge007 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.

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

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

@Koooooo-7Koooooo-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.

Image_20210116
LGTM.

@Koooooo-7

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
jamesgeorge007 requested changes Jan 18, 2021
View reviewed changes

@jamesgeorge007jamesgeorge007 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.

Looking good, left a couple of suggestions.

Comment threadREADME.md Outdated
Comment threadlib/commands/generate.js Outdated

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

@jamesgeorge007jamesgeorge007Jan 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.

Suggested change
console.log(chalk.green(`Generate sidebar file '${sidebar}' success.`))
console.log(chalk.green(`Successfully generated the sidebar file '${sidebar}'.`))

@jamesgeorge007jamesgeorge007Jan 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.

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.jsconsterror=(msg)=>console.error(chalk.red(msg));

@sy-recordssy-recordsJan 19, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Comment threadlib/commands/generate.js Outdated
Comment threadlib/commands/generate.js Outdated
Comment threadlib/commands/generate.js Outdated
Comment threade2e/commands/generate.test.js
@sy-records@jamesgeorge007
Apply suggestions from code review
916479a
Co-authored-by: James George <jamesgeorge998001@gmail.com>
@sy-records
sy-records merged commit e83bfcb into masterJan 22, 2021
@sy-records
sy-records deleted the gen branch January 22, 2021 05:24
This was referenced Mar 15, 2021
Merged
Merged
Closed
@AnilopleAnilople mentioned this pull request Nov 4, 2021
Closed
@trusktrtrusktr mentioned this pull request Jan 26, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Koooooo-7Koooooo-7Koooooo-7 approved these changes
@jamesgeorge007jamesgeorge007jamesgeorge007 approved these changes
@anikethsahaanikethsahaanikethsaha 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

@sy-records@Koooooo-7@jamesgeorge007@anikethsaha