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

Add logo and update brand colors #698

Merged
merged 25 commits into from Jun 8, 2022
Merged

Add logo and update brand colors #698

merged 25 commits into from Jun 8, 2022

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Jun 4, 2022

This makes a few updates to the branding of the theme, here are some major changes:

  • Updates the logo to use number 4 from Move away from pandas styling of the default (demo) theme #373 as it seemed like there was general happiness on that one
  • Updates the primary color to use the blue from the pydata logo (closes closes Move away from pandas styling of the default (demo) theme #373)
  • Also adds a "secondary" color variable with the orange from the pydata logo (thoughts on this? where else could we use the secondary color? or should we not use it at all?)
  • Updates the note admonition to use primary color by default, and warning admonition to use secondary color by default
  • moves the version switcher example (and in our docs) to navbar_start (closes Position and default styling for version switcher #680)
  • Updates the version switcher to use the theme's primary color by default
  • Fix a bug where the toc active link color was using an undefined variable.
  • Adds a dark logo and cleans up some of the logo code

Todo

@choldgraf choldgraf changed the title Add logo and colors Add logo and update brand colors Jun 4, 2022
@jarrodmillman jarrodmillman added this to the 0.9 milestone Jun 4, 2022
@12rambau
Copy link
Collaborator

12rambau commented Jun 4, 2022

what would you think adding a favicon as well ?
#415

@choldgraf
Copy link
Collaborator Author

good point! added favicon

@12rambau
Copy link
Collaborator

12rambau commented Jun 4, 2022

Also I was looking at the final layout and the topleft navbar seems a bit empty to me. What would you think about adding the name "Pydata Sphinx theme" in the logo? maybe using the same colors as the logo and eventually the pydata font (if you know it)

@choldgraf
Copy link
Collaborator Author

Yep i agree! I didn't do it because it is not yet possible, but I hope soon will be 🙂

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 5, 2022

@12rambau I merged your light/dark logo PR so that I could try it out here! That uncovered a couple of bugs which I believe I've addressed in this PR as well.

At the least I think we should add a test for:

  • html_logo, html_title, and html_theme_options.dark_logo specified

Question to resolve

After poking around a bit, I realized that we need to standardize the logo text behavior a bit. This is because Sphinx doesn't tell us whether the value of html_title is manually set by the user, or automatically set by Sphinx. Apparently it always exists. So if we do a check for does html_title exist? it will always be True.

Proposal

What if we defined a top level logo section of our theme options, and it would have three fields: logo_text, logo_image_light, and logo_image_dark to complement logo_link. Text defines the text to the right of the logo, and image-light and image-dark define paths to the light/dark images (these would re-use html_logo if not specified).

So a fully-specified logo config would be:

html_theme_options = {
    "logo_text": "My docs",
    "logo_image_light": "myimage.png",
    "logo_image_dark": "mydarkimage.png",
    }
}

See the latest commit for an example of this config structure in action, I believe it now works as-expected with the text only showing up if it is explicitly set in html_theme_options["logo_text"]

@choldgraf
Copy link
Collaborator Author

I think this one is ready for another review from my perspective

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 5, 2022

@12rambau
Copy link
Collaborator

12rambau commented Jun 5, 2022

At the least I think we should add a test for html_logo, html_title, and html_theme_options.dark_logo specified

Agreed

I'm also in favor of the "logo" section in html_theme_option I think it will be easier to document and html_link and logo will only be used as backup if these values are not set.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 5, 2022

Added a diagonal logo so others could take a look themselves, since it seems popular in #701

So you think it'd be better to use something like:

html_theme_options = {
    "logo": {
        "text": "My docs",
        "image_light": "myimage.png",
        "image_dark": "mydarkimage.png",
    }
}

over

html_theme_options = {
    "logo_text": "My docs",
    "logo_image_light": "myimage.png",
    "logo_image_dark": "mydarkimage.png",
    }
}

I think it would be conceptually simpler, though we'd have to do a deprecation on logo_link I think

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 6, 2022

Latest updates the version switcher style to be less opinionated, that also means it is now responsive with the theme!

image

@jarrodmillman
Copy link
Collaborator

@choldgraf @12rambau After you fix these tests and merge this PR, I believe we are ready to release 0.9. Let me know if there is anything else I should wait for..

@choldgraf
Copy link
Collaborator Author

As long as there aren't any objections, I'll plan to move all of the logo config under a single logo key, and then this one should be ready to go

@choldgraf
Copy link
Collaborator Author

OK I think the latest commit adds support for a dictionary for the logo, and also adds a deprecationwarning for the logo_link. If that looks OK then I think this is good to go!

@choldgraf
Copy link
Collaborator Author

(my 2 cents is that the codecov diff is OK, it is just for the deprecation warning we added + a type check)

drammock and others added 2 commits June 7, 2022 08:52
@jarrodmillman
Copy link
Collaborator

jarrodmillman commented Jun 7, 2022

@choldgraf See #703.

* main:
  Use myst-parser 0.18 (pydata#703)
  Style the toctree (pydata#693)
@jarrodmillman
Copy link
Collaborator

@choldgraf I merged main into your branch to get the tests to hopefully pass. I think this is ready to merge (assuming tests all pass).

@jarrodmillman
Copy link
Collaborator

@choldgraf @12rambau I think this is ready to merge. Once that is done, I plan to make the 0.9 release. Please let me know, if there is any reason to delay a bit. I will be around for the next few weeks, if we need to make a 0.9.1 or (even) a 0.10 release.

@12rambau
Copy link
Collaborator

12rambau commented Jun 8, 2022

The modification are super cool, I really like the new look and feel of the demo website. I think it's important to test the prepare_html method before merging but after that that's definitely ready to fly

@choldgraf
Copy link
Collaborator Author

We do test that method, there are just a couple of lines inside of it that we don't test:

  • The deprecated logo_link warning that is raised if somebody provides logo_link (we will remove this code entirely in a couple of releases)
  • The warning that is raised if somebody provides a non-dictionary value for logo, which I suspect will be a very rare occurrence

@12rambau
Copy link
Collaborator

12rambau commented Jun 8, 2022

then let's fly !

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.

Position and default styling for version switcher Move away from pandas styling of the default (demo) theme
4 participants