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

Add a docs-theme table for rendering methods #4120

Merged
merged 3 commits into from
May 22, 2020

Conversation

alfatimfisher
Copy link
Contributor

@alfatimfisher alfatimfisher commented May 12, 2020

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Add a table to docs-theme render methods

Use a @method tag to render methods

Reviewers should focus on:

Similar functionality to the interface table but for methods

Screenshot

Before:

Screen Shot 2020-05-22 at 11 19 17

After:

Screen Shot 2020-05-22 at 11 47 03

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Can you add one usage of this new feature to docs app so that it's actually used in production and we can test the results easily?

@@ -1,5 +1,5 @@
/*
* Copyright 2017 Palantir Technologies, Inc. All rights reserved.
* Copyright 2020 Palantir Technologies, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this doesn't need to be updated every time a file is edited, it's supposed to show the year when it was created. Please revert this change and the other similar ones in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted these thanks

@alfatimfisher
Copy link
Contributor Author

alfatimfisher commented May 22, 2020

Thanks for the PR! Can you add one usage of this new feature to docs app so that it's actually used in production and we can test the results easily?

@adidahiya Thanks for the feedback adidahiya, I have added some examples for the Table methods https://32525-71939872-gh.circle-artifacts.com/0/packages/docs-app/dist/index.html#table/api.instance-methods and added some logic to include class methods

@@ -80,8 +80,7 @@ function transformDocumentalistData(key, value) {
// reverse the list so highest version is first (easier indexing)
return Array.from(majors.values()).reverse();
}
// remove all class declarations as they're currently unused and only increase bundle size
if (value != null && value.kind !== "class") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've had to change this to include the class members but there might be a more efficient way to do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine

@adidahiya
Copy link
Contributor

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

table docs changes look great, thanks!

@@ -80,8 +80,7 @@ function transformDocumentalistData(key, value) {
// reverse the list so highest version is first (easier indexing)
return Array.from(majors.values()).reverse();
}
// remove all class declarations as they're currently unused and only increase bundle size
if (value != null && value.kind !== "class") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine

@adidahiya adidahiya merged commit 9df8576 into palantir:develop May 22, 2020
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.

None yet

2 participants