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

Default theme option #804

Merged
merged 4 commits into from
Oct 30, 2018
Merged

Default theme option #804

merged 4 commits into from
Oct 30, 2018

Conversation

Bassetts
Copy link
Contributor

Opening a PR to initiate discussion.

Everything is there and working, but there are a few things I dislike/am unsure of.

  1. Calling this option default-theme is confusing to the user. The options are presented as 'themes' to the visitor in the theme popup, but from a user's perspective the theme is the whole theme inside src\theme. Maybe this should be default-style or default-theme-style? Maybe I just need to explain the option better in the documentation?
  2. I created a new handlebars helper to append (default) to the selected default theme in the theme popup. Originally I had done this in the javascript that sets the theme on page load, but it seemed a bit hacky when we know which theme is the default during render. Not sure if this is the best way, or best implementation of a helper though.
  3. I have left the ordering of the theme popup as it is, but this means the default may now not be the top item. Should we re-order the list of themes to always have the default as the first item?

Once complete this will resolve #95 and #713

@Michael-F-Bryan
Copy link
Contributor

Calling this option default-theme is confusing to the user

I think default-theme should be okay. People will probably be more familiar with the drop-down than the theme/ directory, and someone playing around with theme/ should understand the difference. Maybe we could update the documentation to mention this only affects the colour schemes selected by default from the themes drop-down, and is totally separate from the theme/ directory?

@Bassetts
Copy link
Contributor Author

@Michael-F-Bryan realised I forgot to comment. I have updated the documentation to specify this affects the themes drop-down.

@Michael-F-Bryan Michael-F-Bryan merged commit 42b87e0 into rust-lang:master Oct 30, 2018
@Michael-F-Bryan
Copy link
Contributor

Merged 🎉 Thanks for all the hard work @Bassetts!

@fitzgen
Copy link
Member

fitzgen commented May 17, 2019

Awesome! 🎉

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
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.

Select default theme
3 participants