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

Lowercase matching the theme name #1079

Merged
merged 2 commits into from
Nov 5, 2019

Conversation

deg4uss3r
Copy link
Contributor

Using .to_lowercase() on the theme matching to avoid needing exact capitalization in the book.toml. Fixes #1078

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned that this would make it impossible to use a theme that actually has an uppercase character in it. However, that seems unlikely to me.

src/renderer/html_handlebars/hbs_renderer.rs Outdated Show resolved Hide resolved
@deg4uss3r
Copy link
Contributor Author

@ehuss Doesn't the PR #804 get the themes and then lower case them which is why this is necessary?

https://github.com/rust-lang/mdBook/blob/master/src/renderer/html_handlebars/helpers/theme.rs#L23

@Dylan-DPC-zz Dylan-DPC-zz merged commit 3ea0f9b into rust-lang:master Nov 5, 2019
@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2019

@deg4uss3r I think that lowercase only applies to showing the (default) indicator. The CSS class name is used directly as-is, and I believe those are case sensitive.

@deg4uss3r
Copy link
Contributor Author

@ehuss Ah, I think you're right, here seems to be the real cause to my confusion making the list of themes capitalized which is what I put into the config, rather than the true matching lowercase value.

Do you think this solution interferes with the actual style sheet selection? If so, I'm happy to rework it to get the config option to be case-insensitive if I can be pointed in the right direction.

@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2019

Do you think this solution interferes with the actual style sheet selection? If so, I'm happy to rework it to get the config option to be case-insensitive if I can be pointed in the right direction.

No, I think it's fine. Adding a theme is a nontrivial task, and I don't suspect people use capitalized css names much anyways.

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
* Using .to_lowercase() on the theme matching to avoid needed exact capitolization in the book.toml

* Changed the default-dark-theme from .to_string() to .to_lowercase() to match theme
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.

Non light Default Theme Isn't Served On Page Load
3 participants