-
-
Notifications
You must be signed in to change notification settings - Fork 750
Convert default themes to JSX; merge into typedoc repo #1634
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
Conversation
|
|
||
| get isExported() { | ||
| return TODO as boolean; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was being called from templates but not declared anywhere. Do you know what the implementation should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was removed in 0.20, it's always true now.
|
It works! Tests have been (temporarily) updated to emit prettified versions of both the spec html and the typedoc output for visual inspection. These outputs match: Next I'm going to render the spec output from |
|
Notes about this PR: it is currently taking the expected and the actual html and formatting both with prettier and some find-and-replace. We'll need to remove that and replace all the specs prior to merging this. Tests run way slower doing this postprocessing. In a handful of places, markdown is wrapped in an |
|
Also need to figure out how and when the webpack configs run; wire them into the build. |
|
TODO use osnap instead of percy for visual testing |
|
Added scripts for visual regression testing. Pushed the generated report here: https://github.com/cspotcode/typedoc/tree/jsx-visual-regression-report
Open |
|
I updated the op with a checklist of tasks. |
Gerrit0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decide where theme source assets should live, currently in /src/lib/output/themes/
decide where built theme assets should live, currently in /dist/lib/output/themes/bin
I'm fine with this, can always change later... not a public path.
they are only used when logging an error message: can we remove the info from the message?
I think it is still useful, will likely be moving eventually once I get around to creating a proper comment parser that's a bit smarter than regex.
fix isExported to match review feedback?
Likely just going to end up removing this
| } else { | ||
| return contents; | ||
| } | ||
| // if (path.substr(-4).toLocaleLowerCase() === ".hbs") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "module": "CommonJS", | |||
| "lib": ["ES2018"], | |||
| "lib": ["ES2018", "dom"], | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hoping to avoid needing to do this... probably time typedoc started using project references. The themes have dom, but not typedoc itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to publish the themes as separate packages with a simple monorepo setup? It would make the themes more clearly look like how a third-party theme might be implemented.
| export class Demo { | ||
| private foo: number; | ||
|
|
||
| //@ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was complaining that props is never used. I could add the same this.foo trick for this.props
cspotcode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are only used when logging an error message: can we remove the info from the message?
I think it is still useful, will likely be moving eventually once I get around to creating a proper comment parser that's a bit smarter than regex.
Ok, might need to refactor DefaultThemeRenderContext a bit. The current implementation uses a singleton RenderContext for each page, but we can instead create a new RenderContext for every page, so that the RenderContext has information about the page -- maybe a reference to the PageEvent? -- which it can expose to the partials.
If we're going to create a new RenderContext for each page, then we can reduce the work being done in the constructor. Instead of each partial being a factory wrapper around the partial -- (bindings: Bindings) => (reflection: Reflection) => <> -- we can simplify to a single function which accepts both bindings and reflection.
I had thought of inlining all partials as methods of the RenderContext, but I feel like splitting into separate files is easier to follow. I dunno, I'm not sure which is better.
| export class Demo { | ||
| private foo: number; | ||
|
|
||
| //@ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was complaining that props is never used. I could add the same this.foo trick for this.props
|
I'm rather confused as to why GitHub didn't pick this up as merged, it's been merged and now released as 0.22.0! Thanks again :) |
Replacing TypeStrong/typedoc-default-themes#137
EDIT next tasks to get this PR ready to merge:
/src/lib/output/themes/npm run build?webpack.config.jsfiles?/dist/lib/output/themes/bin<md>emit; emit a div insteadexpected/actualfromrenderer.test.tsspecswith the JSX output__partials__into an instance ofDefaultThemePartialsDefaultTheme; wrap all partials in factory that binds them to the theme / partials.MarkedPlugininjectionMarkedPlugin.sourcesandMarkedPlugin.outputFileNameMarkedPluginandLayoutPluginto not be plugins; merge with ThemeMarkedHelpersMarkedPluginas a plugin, grabbing it viagetComponentisExportedto match review feedback?hack()inPartialsFuture work
Not necessary in this PR, but it's nice to get excited about future possibilities:
<spans>: {, [, ], etc.<></>fragment wrappers,{}wrappers, etc.