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

make it easier to use a different default theme #4278

Merged
merged 2 commits into from Nov 1, 2018

Conversation

@xrmx
Copy link
Contributor

@xrmx xrmx commented Jun 20, 2018

This is for discussion more than inclusion. If this is sound we may want to do the same for default extensions. Our goal is to avoid the conf.py entirely in the documentation repos or make it very slim.

@xrmx xrmx force-pushed the extendconfpyctx branch from b0aa489 to d997818 Jun 26, 2018
Copy link
Member

@ericholscher ericholscher left a comment

This looks like a reasonable change. I agree the theme should be set by a string that's passed in, instead of hard-coded. We'd want this to be configurable by a setting though, if we actually wanted it to be easily changed.

@xrmx xrmx force-pushed the extendconfpyctx branch from d997818 to 96a4b94 Jun 26, 2018
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 26, 2018

Updated PR, haven't touched settings because we can update the context via the finalize_sphinx_context_data signal.

@xrmx xrmx changed the title RFC: make it easier to use a different default theme make it easier to use a different default theme Jun 26, 2018
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jul 18, 2018

ping

@humitos
Copy link
Member

@humitos humitos commented Aug 15, 2018

Thanks for your PR.

We'd want this to be configurable by a setting though, if we actually wanted it to be easily changed.

I suppose this is the only thing missing to be in a "ready to merge" state.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Aug 16, 2018

@humitos can we please merge this as is and if someone want to expose this with a setting just do another PR? I don't have much bandwidth to work on this and I think the code duplication reduction is worth on its own.

@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Aug 16, 2018

That might be a good 'Good First Issue' to open, too.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Nov 1, 2018

Tested this locally and it works well. Agreed the duplication is reason enough, and it can be changed via signal. 👍

@ericholscher ericholscher merged commit e35e5ff into readthedocs:master Nov 1, 2018
1 check passed
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Nov 2, 2018

@ericholscher thank you very much for skimming these old PRs, much appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants