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

upgrade documentalist, add "edit this page" links #2615

Merged
merged 9 commits into from
Jun 20, 2018
Merged

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Jun 20, 2018

Fixes #101

Changes proposed in this pull request:

  • upgrade to documentalist 1.4.0 and use new NpmPlugin so there's only one docs data file (versions and releases now live inside docs.json)
  • add Documentation renderPageActions prop to customize upper-right actions
    • because docs-theme doesn't know about our github URL
  • add "Edit this page" link in these actions

image

Copy link
Contributor

@themadcreator themadcreator left a comment

Choose a reason for hiding this comment

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

Some nits and questions, otherwise approved

private renderPageActions(page: IPageData) {
return (
<AnchorButton
href={`https://github.com/palantir/blueprint/blob/develop/${page.sourcePath}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

probably worth extracting a const

}
const version = this.props.useNextVersion && pkg.nextVersion ? pkg.nextVersion : pkg.version;
return (
<a className={Classes.TEXT_MUTED} href={`https://www.npmjs.com/package/${pkg.name}`} target="_blank">
Copy link
Contributor

Choose a reason for hiding this comment

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

const

@@ -100,3 +99,8 @@ export class NavHeader extends React.PureComponent<INavHeaderProps, {}> {
this.props.onToggleDark(!this.props.useDarkTheme);
};
}

/** Get major component of semver string. */
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

await generateDocumentalistData();
generateReleasesData();
generateVersionsData();
} catch (err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

comment why console.log is better than console.error

// one major version per release
const majors = new Map();
for (version of value) {
const major = semver.major(version)
Copy link
Contributor

Choose a reason for hiding this comment

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

you just added that other function to extract this. should we consolidate to use one method or the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this runs in node at build time, where we have the actual semver package. the other function runs at render time on the client where we don't have (or need) that external package.

@blueprint-bot
Copy link

script comment

Preview: documentation | landing | table

@giladgray giladgray merged commit f052781 into develop Jun 20, 2018
@giladgray giladgray deleted the gg/dm-npm-plugin branch June 20, 2018 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants