Few fixes for multiple themes support feature #47686
Conversation
| @@ -197,7 +197,7 @@ a.test-arrow { | |||
|
|
|||
| #help dt { | |||
| border-color: #bfbfbf; | |||
| background: #fff; | |||
| background: rgba(0,0,0,0); | |||
There was a problem hiding this comment.
This creates black-on-black text for the keyboard shortcut keys on the help window.
There was a problem hiding this comment.
Ohh, i didn't realize that the help window keys got switched over to <kbd> tags. This is fine.
| padding: 4px; | ||
| text-align: center; | ||
| background: rgba(0,0,0,0); |
There was a problem hiding this comment.
Pretty sure there's a conversation somewhere about where the colors need to go, since you recently made sure every color directive went into the theme css. Or did you want the theme-picker button to look the same in every theme? >_>
There was a problem hiding this comment.
I want to the background of the button to be invisible so that the buttons background is consistent to its parent. It's the only case where I'll put colors in this file.
There was a problem hiding this comment.
Ohhhh, no alpha, now i get it.
| @@ -364,16 +364,20 @@ kbd { | |||
| background: #f0f0f0; | |||
| } | |||
|
|
|||
| #theme-picker:hover { | |||
There was a problem hiding this comment.
Can you add the same style for :focus? Currently, you can't tell if the button is focused or not.
| } | ||
|
|
||
| #theme-choices > div:hover { | ||
| #theme-choices > button:hover { |
There was a problem hiding this comment.
Same thing regarding :focus as above.
| } | ||
|
|
||
| #theme-choices > div:hover { | ||
| #theme-choices > button:hover { |
| border: 1px solid; | ||
| border-radius: 3px; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| #theme-picker:hover { |
There was a problem hiding this comment.
Should be in main.css actually. Strange...
kennytm
added
the
S-waiting-on-review
GuillaumeGomez
force-pushed
the
theme-fixes
branch
from
8021624
to
e75d9e8
Compare
GuillaumeGomez
force-pushed
the
theme-fixes
branch
from
e75d9e8
to
e78f139
Compare
| @@ -74,10 +74,12 @@ r##"<!DOCTYPE html> | |||
| {sidebar} | |||
| </nav> | |||
|
|
|||
| <button id="theme-picker"> | |||
| <img src="{root_path}brush.svg" width="18" alt="Pick another theme!"> | |||
| <div class="theme-picker"> | |||
There was a problem hiding this comment.
do we know why this change makes it work? semantically, this should be a <button>.
There was a problem hiding this comment.
Now it's a <button> inside this <div> so i would assume that there's some layout trickery going on.
There was a problem hiding this comment.
Just like @QuietMisdreavus said. :) Before it was buttons inside a button, now it's buttons inside a div. More logical for everyone (browsers included).
There was a problem hiding this comment.
Oh right, i was missing the other <button>s being put inside this one. I like this version more.
|
Looks good to me! Thanks for the quick follow-up! @bors r+ |
|
📌 Commit e78f139 has been approved by |
|
@bors p=1 The theme selector is broken in Firefox right now, and this fixes that, so let's give it a little push. |
|
⌛ Testing commit e78f139 with merge e86fd3fb77f10efbb172f185629bc1b83779e6e5... |
|
💔 Test failed - status-appveyor |
|
@bors retry - appveyor timed out |
Few fixes for multiple themes support feature r? @QuietMisdreavus Fixes #47695.
|
☀️ Test successful - status-appveyor, status-travis |
r? @QuietMisdreavus
Fixes #47695.