-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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 button to toggle the optional theme when viewing the docs #13463
Conversation
I'm not sure the button's placement is right there. I mean, I don't think a lot of people do actually scoll to the bottom of the /css and /components pages. |
@XhmikosR It's |
Indeed... I missed it because the button is covered by the browser info when loading stuff. Maybe right would be better? |
Didn't want to risk interfering with the side nav, but I don't really care about the exact placement myself. Willing to change it to whatever is desired. |
Maybe just merge it as it is then and we can change it later if needed. @mdo: thoughts? |
@@ -15,6 +15,10 @@ | |||
|
|||
<!-- Bootstrap core CSS --> | |||
<link href="../dist/css/bootstrap.min.css" rel="stylesheet"> | |||
{% if page.slug == "css" or page.slug == "components" or page.slug == "js" %} | |||
<!-- Optional Bootstrap Theme --> | |||
<link href="data:text/css;charset=utf-8," data-href="../dist/css/bootstrap-theme.min.css" rel="stylesheet" id="bs-theme-stylesheet"> |
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.
BTW @cvrebert is this right? I mean is the href="data:text/css;charset=utf-8,"
part needed?
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.
Yes, the <link>
has to have an href
for our HTML to validate.
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 thought so too. But why do we need data-href
too? I mean, we should be able to change href
just fine, shouldn't we?
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.
Right, but we need to put the actual URL to the theme CSS somewhere in our code (so that we can then put it into or take it out of the href
). This way (data-href
) seemed cleaner than hardcoding the URL into the JavaScript.
Conflicts: docs/assets/css/docs.min.css
@mdo So, any feedback on the button placement, styling, or phrasing? |
Yeah I'll be taking a crack at this shortly. Have some local changes to tweak it. I'm hesitant to do a button in the bottom left, so I might just make this a link below the right subnav. Not ideal, but neither is the floating button. We can always improve this later, too. |
Conflicts: dist/css/bootstrap.css.map dist/css/bootstrap.min.css docs/assets/css/docs.min.css docs/dist/css/bootstrap.css.map docs/dist/css/bootstrap.min.css
add button to toggle the optional theme when viewing the docs
<3 |
Fixes #9764.
The button is only present on relevant pages of the docs. I didn't notice any undesirable effects on docs-specific styling.
/cc @twbs/team for review.