-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Clean all superposed white backgrounds #1657
Conversation
@skjnldsv, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jancborchardt, @MorrisJobke and @DeepDiver1975 to be potential reviewers. |
@@ -215,7 +213,7 @@ table th:focus .sort-indicator.hidden { | |||
|
|||
table th, | |||
table td { | |||
border-bottom: 1px solid #eee; | |||
border-bottom: 1px solid 1px solid rgba(0,0,0,0.08); |
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.
typo? 😉
@@ -439,8 +438,7 @@ | |||
width: 27%; | |||
min-width: 300px; | |||
display: block; | |||
background: #fff; | |||
border-left: 1px solid #eee; | |||
border-left: 1px solid 1px solid rgba(0,0,0,0.08) |
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.
same here
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.
sigh... Copy/paste wasn't really effective here...
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.
@skjnldsv Nice, very good call on using transparency vs greyspale! Also the code style is rgba(0, 0, 0, .08)
(space after comma and no 0 before dot ;)
502a0d0
to
5109648
Compare
@@ -144,7 +142,7 @@ | |||
#filestable tbody tr.searchresult, | |||
table tr.mouseOver td { | |||
transition: background-color 0.3s ease; | |||
background-color: #f8f8f8; | |||
background-color: rgba(0,0,0,0.08); |
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.
Compare this with line 216 where you use the ame .08 color for #eee, instead of #f8f8f8 like here. ;) They should be different, no?
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.
It should! To be honest, i'm not calculating the hex to rgba! :p
It's a visual estimation ^^
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 will calculate the new value. I like it more precise! :)
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.
Done @jancborchardt .
Values are now calculated base on a rgba transparency on white background.
9f8746b
to
372b2b1
Compare
372b2b1
to
8e68407
Compare
3cda90b
to
b1e3fc0
Compare
1c6ac20
to
3215fad
Compare
outline: 0; | ||
padding-right: 24px !important; | ||
} | ||
|
||
select:hover { | ||
background-color: #fefefe; |
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.
@jancborchardt I'm not really sure this is humanly visible 😆
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.
Probably not really. Do you want to adjust this to the equivalent of #fff instead? Or rather it really needs to be solid #fff because by default the select is grey and it could be on top of dark background.
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.
We shouldn't think of dark backgrounds superposition. Because the rgba I'm changing are planned on a #fff bg. If we want a dark background with the same contrast, we should reverse the values. :)
For this line, I think we could just remove it. Or choose a completely different value to set for all inputs:hover.
@@ -272,8 +272,8 @@ button:hover, button:focus, | |||
.button:hover, .button:focus, | |||
.button a:focus, | |||
select:hover, select:focus, select:active { | |||
background-color: rgba(255, 255, 255, .95); |
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.
@jancborchardt same here. This is not visible to the eye. I changed to something more visible. What do you think?
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.
The point about this was that the background color shines a bit through. For example when we have it in the header of the link share page there are buttons on the top right.
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.
Okay, makes sense! :)
Current coverage is 56.23% (diff: 100%)@@ master #1657 diff @@
==========================================
Files 1070 1070
Lines 60505 60505
Methods 6829 6829
Messages 0 0
Branches 0 0
==========================================
Hits 34027 34027
Misses 26478 26478
Partials 0 0
|
} | ||
tbody a { color:#000; } |
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.
We don't need to redefine the a colors since it's already set on core/styles.css
@skjnldsv you need to "sign off" all your commits: Can you fix all your commit messages please? |
I know, I forgot, I will when I will squash those commits :) |
@georgehrke Yep, the login form don't have a background. We should apply one to it for this case only. :) |
@skjnldsv can you fix what @georgehrke mentioned and the other things? Then squash or so, and we can review it. :) |
@jancborchardt Not now. I'm lost for now. This pr extends to more than just white backgrounds I think. |
@skjnldsv I’m not on IRC atm ;) either Github or email. |
Just sent you one 🚀 |
@skjnldsv What is the status of this? |
@MorrisJobke on hold while I wait for the scss pr. Too much sh*** to clean. |
See nextcloud/server#1657 Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
see nextcloud/server#1657 Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
In nextcloud there's too much elements with a white background. If the parent element already have a white bg, we should reapply one. This causes a lot of difficulties for themers to create their own.
We should also use transparents borders so we can easily change background without breaking the contrast between bg and borders.
The though came from my work on a dark theme. Changing the body bgcolor isn't enough.