Skip to content

Conversation

@CsatariGergely
Copy link
Contributor

This pr adds the possibility to change the background
of the search area.

Signed-off-by: Gergely Csatari [email protected]

@Blendify Blendify self-requested a review December 12, 2018 02:04
@davidfischer
Copy link
Contributor

Is this the same as #404?

@Blendify
Copy link
Member

Yes but a different approach.

@jessetan
Copy link
Contributor

Assuming there won't be many of these style overrides, I like this approach more than the one in #404.

Two things I'm not certain on:

  • Do we need to prefix this option with something like style_ so that we can group future style overrides, or do we think we will not have any more?
  • This allows changing of the CSS background-color property. Would CSS background be a better choice so one can do background images or gradients as well?

@Blendify
Copy link
Member

Assuming there won't be many of these style overrides, I like this approach more than the one in #404.

Two things I'm not certain on:

  • Do we need to prefix this option with something like style_ so that we can group future style overrides, or do we think we will not have any more?
  • This allows changing of the CSS background-color property. Would CSS background be a better choice so one can do background images or gradients as well?
  1. Yes I think we should prefix this.
  2. I think so, it gives more freedom.

Also making it to modify the background css property instead of
background-color

Signed-off-by: Gergely Csatari <[email protected]>
@CsatariGergely
Copy link
Contributor Author

Thanks for the suggestions @jessetan and @Blendify I've applied both of them.

@jessetan
Copy link
Contributor

jessetan commented Jan 3, 2019

Code looks OK, just needs some documentation here: https://github.com/rtfd/sphinx_rtd_theme/blob/master/docs/configuring.rst#html-theme-options

@CsatariGergely
Copy link
Contributor Author

@jessetan documentation is added.

@jessetan
Copy link
Contributor

jessetan commented Jan 7, 2019

LGTM

@CsatariGergely
Copy link
Contributor Author

What else is needed to merge this?

@CsatariGergely
Copy link
Contributor Author

@ericholscher , @agjohnson can you please take a look please?

@ericholscher
Copy link
Member

This feels oddly specific. Is there a reason we'd only support this one setting for a background color? It feels like having the theme be more overridable in general is a good goal, but this is a half measure. The comments on #404 touch on this, but having a full set of colors that can be overridden feels like the proper solution.

@ericholscher
Copy link
Member

On second thought, this is a good approach for now, but we should do a larger rethink in #404.

@ericholscher ericholscher merged commit c2b61e4 into readthedocs:master Feb 12, 2019
@CsatariGergely CsatariGergely deleted the header-background branch February 13, 2019 09:48
@CsatariGergely
Copy link
Contributor Author

Thanks @ericholscher . We can remove this approach once #404 is merged and use a more generic one.

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.

5 participants