Skip to content

Fix theme.js in custom themes #502

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

Closed
wants to merge 2 commits into from
Closed

Conversation

cpriest
Copy link
Contributor

@cpriest cpriest commented May 14, 2017

I believe this (along with the original PR#252) fully corrects the original issue #227 as well as the regression #382.

@blakeembrey
Copy link
Member

Can you explain how this fixes the issue? In the linked PR I did, the code doing this sort of behaviour was already commented out and this change is not consistent with that commented out behaviour.

@cpriest
Copy link
Contributor Author

cpriest commented May 15, 2017

Sure, I don't have much historical knowledge of the project but I believe this part of your commit:

+        this.addDirectory('default', Renderer.getDefaultTheme());
+        this.addDirectory('theme', theme.basePath);

Fixed the 'fallback' part which may not have ever been working properly. I essentially took the other part of your commit, re-instituted the code and fixed it so that it would work properly for loading the theme.js file.

I also tested (manually) both scenarios regarding loading template files with or without a theme.js present.

This allows for non-whitespace characters in a tag name.  This
specifically does not correct for inline tags such as {@link http...}
which should probably be handled differently.
@cpriest
Copy link
Contributor Author

cpriest commented May 15, 2017

Shoot I did not mean for this to come into this same PR. LMK what you'd like me to do here...

Close this one and submit them separately? I'm not sure there is a way to pull this commit out of the PR, if you know of a way, let me know and I will.

@blakeembrey
Copy link
Member

You can use rebase on the branch and fix it up.

I'd also prefer that you use const over let for the PR, so feel free to also fix that up. I'd also say that class names should always use PascalCase, so that change might be useful. I'm not 100% on using default always - maybe we should check default before using it or use the old behaviour of eval?

@cpriest
Copy link
Contributor Author

cpriest commented May 15, 2017

I ran the tests on my local machine and they all passed, I think Travis was running slow or something.

@cpriest
Copy link
Contributor Author

cpriest commented May 15, 2017

@blakeembrey I think that may be why it was commented out as it didn't work with eval when tested against the typedoc-theme-markdown. I based the fix on export default which is what the markdown theme had.

If you didn't want to use the default, we could make it the same as for plugins. The require() should pass back a function which accepts the app. Thoughts?

I'll fix them up and re-submit.

@blakeembrey
Copy link
Member

blakeembrey commented May 15, 2017

Sure, but we can make them both consistent by including a check like typeof x === 'function' ? x : x.default. I think that'd be the best compromise to enable both use-cases for now.

Yeah, Travis sometimes has issues with the timing. Might need to increase it sometime, usually I just restart it.

cpriest added a commit to cpriest/typedoc that referenced this pull request May 16, 2017
…iginal issue TypeStrong#227 as well as the regression TypeStrong#382.

Also addresses the comments from PR TypeStrong#502, now closed.
@cpriest cpriest closed this May 16, 2017
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.

2 participants