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

Allow setting the nav header color via config #228

Closed
wants to merge 1 commit into from

Conversation

asakusuma
Copy link

If you have a collection of related, but distinct doc sites, it's really nice to be able to color code them without having a completely custom theme for each.

@mikhail
Copy link

mikhail commented Jul 29, 2015

Would love to see this, but I expected the appropriate change in the css file. Does it not work the same?

@asakusuma
Copy link
Author

Are you saying you want the config to be applied via stylesheet and not inline style?

@mikhail
Copy link

mikhail commented Jul 29, 2015

Right, that way it'd be applied to all places that use #2980B9 and would be consistent. I don't know how the templating works here with css

@asakusuma
Copy link
Author

Well the purpose of this PR is not to allow people to globally change a color, but to specifically set the header color. It would be cool to allow a global change, but I'm not sure if that's possible since, as far as I'm aware, sphinx theme packages are comprised of css and not sass, and I'm not aware of a way to modify a theme css file based on the config.

@asakusuma
Copy link
Author

If it is in fact possbile and someone could explain how, I would be happy to update the PR.

@snide
Copy link
Collaborator

snide commented Jul 29, 2015

So I haven't worked on this project in a bit, so I'll leave it to someone else one of the maintainers to make a decision but thought I'd chime in here.

The sass files give you essentially what you are looking for. An ability to theme the look and feel of a project using variables. Using python to change frontend is generally backwards way to do it and not the right tool for the job. Sphinx took this in the wrong direction by allowing the css_t stuff which essentially is a junk version of sass or any other preprocessor which are pretty standard tools at this point. Using config values to manage css is a very lazy (and dirty) way to solve a problem.

I generally think it's a bad idea for python code to modify the frontend when the frotend world has perfectly good ways to handle these problems.

To change colors in the proper way you want simply edit the sass files. Look to PR #226 as an example of how coloring should be handled with the source files. But yeah, you want to edit sass files, then build css. Not edit python files to create style tags or run a secondary, un-used outside of sphinx css_t system.

@asakusuma
Copy link
Author

I agree that it's generally a bad idea for python code to modify the frontend.

However, I think changing just the header color via config is akin to setting the logo image via config. It's something super simple that allows you to individualize the site without having to do any dev work. This is how a lot of wordpress themes work, you can't just re-skin your theme through the config, but you can change a couple of key visual things, like the logo image and a header color.

Regardless, we are using my own fork to handle this, so it's not a big deal if other people don't think this is a good idea.

@mikhail
Copy link

mikhail commented Jul 29, 2015

@snide I'm not sure what you mean... Isn't this exactly what the TODO is describing?

As a general rule I don't want to modify the theme. I just want to configure it.

@asakusuma I definitely want to see this, as color is the most obvious personalization on this theme

@snide
Copy link
Collaborator

snide commented Jul 29, 2015

@siminm That todo actually describes what was done in this PR.

#226

@ghost ghost mentioned this pull request Jul 30, 2015
@agjohnson
Copy link
Collaborator

I'm -1 on this too. This would be better left off to css overrides.

@agjohnson
Copy link
Collaborator

Closing this out due to inactivity and because of the issues raised above -- I don't think this PR is mergable. I think this use case is better served with the other options that were outlined above and I wouldn't want to start implementing changes as inline CSS styles.

@agjohnson agjohnson closed this Sep 9, 2015
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.

4 participants