Feature/api docs #79
Feature/api docs #79
Conversation
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 31 32 +1
Lines 187 199 +12
Branches 6 6
=====================================
+ Hits 187 199 +12
Continue to review full report at Codecov.
|
| } | ||
| } | ||
| return md; | ||
| } |
There was a problem hiding this comment.
could possibly use for of?
for (const file of files) {
if (shouldDocumentComponent(file)) {
md += await generateApiForFile(file);
}
}
There was a problem hiding this comment.
I did originally but got this from eslint:
iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations. (no-restricted-syntax)
discussion airbnb/javascript#1271
| if (!Component) { | ||
| console.log(chalk.red("Error loading component:"), file); | ||
| } | ||
|
|
There was a problem hiding this comment.
is it necessary to have two if (!Component)?
There was a problem hiding this comment.
second is "if there still isn't a component after loading the default", so yes
There was a problem hiding this comment.
- try load example.js
- if this didn't load, index.js
- if this isn't a component, look for default export
- if still no component then log
| if (!props) { | ||
| return ""; | ||
| } | ||
|
|
There was a problem hiding this comment.
maybe picky, but perhaps move the if !props, return to the top to avoid the const title = "Props"; from being defined
There was a problem hiding this comment.
unless need the title there anyway
| let newString = ""; | ||
| for (let i = 0; i < length; i += 1) { | ||
| newString += string; | ||
| } |
There was a problem hiding this comment.
could maybe use for of here too?
| import { | ||
| BUTTON_COLOUR, | ||
| BUTTON_COLOUR_DARKEN_15, | ||
| WHITE, | ||
| YELLOW | ||
| } from "govuk-colours"; | ||
|
|
||
| const Button = glamorous.button( | ||
| const GButton = glamorous.button( |
There was a problem hiding this comment.
was this supposed to be renamed?
There was a problem hiding this comment.
ah looks like it, although the naming is confusing. Maybe GlamorousButton? 👨💻
There was a problem hiding this comment.
yeah, react-docgen doesn't pick up glamorous components so I'm having to wrap them with a JSX component. I renamed the inner components to start with a G for Glamorous
There was a problem hiding this comment.
okay might be good to document that either inline comments or in the readme
There was a problem hiding this comment.
it's in the PR description and I've created an issue here #81
There was a problem hiding this comment.
I agree it's not great, I just didn't want to spend too much time on documentation at this point, will hopefully revisit soon, maybe there will be progress on glamorous + react-docgen compatibility in the meantime and we can just undo this.
Generate
API.mdby callingnpm run docs.This uses react-docgen.
Due to the default resolver of react-docgen not picking up Glamorous components, I've had to wrap some Glamorous only components with a JSX wrapper. I'd rather not have to do this but not sure of any alternative solution.
reactjs/react-docgen#256
I'm also not sure the format of the current API.md is useful but opening PR for consideration.