Gray out label when disabled #12455
Gray out label when disabled #12455
Conversation
| @@ -131,7 +131,8 @@ class SwitchControl extends ImmutableComponent { | |||
| styles.switchControl__text__label, | |||
| styles.switchControl__text__label_right, | |||
| this.props.small && styles.switchControl__text__label_small, | |||
| this.props.customStyleTextRight | |||
| this.props.customStyleTextRight, | |||
| this.props.disabled && styles.switchControl__text_disabled | |||
There was a problem hiding this comment.
@user512 would you mind moving this.props.customStyleTextRight under this line as it is expected to override the inherited properties?
user512
force-pushed
the
gray-out-text-when-disable
branch
from
740a84d
to
f81c12f
Compare
| @@ -131,6 +131,7 @@ class SwitchControl extends ImmutableComponent { | |||
| styles.switchControl__text__label, | |||
| styles.switchControl__text__label_right, | |||
| this.props.small && styles.switchControl__text__label_small, | |||
| this.props.disabled && styles.switchControl__text_disabled, | |||
There was a problem hiding this comment.
@luixxiul, thank you for the feedback, just fixed as you recommended.
I am not very familiar with styling in this app or styling in React, would you mind telling me how does order affect styling? I tried reading https://github.com/brave/browser-laptop/blob/master/docs/style.md but can't find how order affect styling.
Would be great if you can suggested some reading about styling in React, thanks 😄
There was a problem hiding this comment.
hey @user512. The order affects styles the same way as CSS cascading works. The last rule overrides the previous so in your changes, the disabled interface would override other styles, which is the correct and expected behavior. Nice work!
There was a problem hiding this comment.
maybe this could be the last conditional but I think it's ok as-is. Thanks!
Codecov Report
@@ Coverage Diff @@
## master #12455 +/- ##
=======================================
Coverage 55.83% 55.83%
=======================================
Files 278 278
Lines 26977 26977
Branches 4362 4362
=======================================
Hits 15062 15062
Misses 11915 11915
|
There was a problem hiding this comment.
++ changes look great! 😄
Including @cezaraugusto for more advice- he put together this helpful guide which is in our wiki that you may enjoy:
https://github.com/brave/browser-laptop/wiki/Refactoring-styles-to-Aphrodite
|
@bsclifton @user512 Is this fix only for Flash switch or all switch in general when off? |
|
@srirambv this should be for all places where it's used (in a disabled state). The test plan captures the only known occurrence... but if you set the |
|
@bsclifton none of the switches which are turned off shows gray out. The flash text does it when there is no flash installed on the machine. Otherwise all other text just looks normal. |
|
@srirambv per the test plan, it should only be grayed out when Flash is not installed on the machine (disabled). Because you can't turn something on if it's not installed 😛 So you just verified this is working as expected 😄 👍 |
|
That works but the original issue was intended to show grayed out text when the switch was turned off. So i was expecting to see all text in gray which were turned off. Basically like this #7634 (comment) |
Fix #7634
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
go to about:preferences#plugins without installing Pepper Flash and observe label is gray out.
Reviewer Checklist:
Tests