-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Lowercase matching the theme name #1079
Conversation
…itolization in the book.toml
There was a problem hiding this 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.
@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 |
@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. |
@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. |
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. |
* 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
Using
.to_lowercase()
on the theme matching to avoid needing exact capitalization in thebook.toml
. Fixes #1078