-
Notifications
You must be signed in to change notification settings - Fork 35
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
Remove comments builder #38
Remove comments builder #38
Conversation
The CSS classes have the sphinx prefix |
}) | ||
|
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.
All this file can be removed, I suppose. But the lib/settings.js
was required by the others libs, so it was included in the bundle. Settings.js have information that is used outside the comments builder?
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.
Also, we need to re-minify the js (css too?) after the PR is completed.
I think this PR wasn't ready to be merged, there were some decisions to be made |
|
Ok, I'll continue with this on other PR, also try to test this with my rtd instance |
PR on top of #31
Related PR: readthedocs/readthedocs.org#3802
What is missing?