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

Fix box shadow of header elements #7572

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Fix box shadow of header elements #7572

merged 1 commit into from
Dec 20, 2017

Conversation

MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Dec 19, 2017

  • unify shadow blur from 3px to 10px
  • remove opacity of background of app labels
  • for IE: use box-shadow as fallback (because the filter: drop-shadow is not supported)

Found while reviewing nextcloud/notifications#89

cc @nextcloud/designers

Before (opacity, bigger blur, no shadow on notifications in Safari):

bildschirmfoto 2017-12-19 um 12 10 16

After:

bildschirmfoto 2017-12-19 um 12 09 56

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews design Design, UI, UX, etc. papercut Annoying recurring issue with possibly simple fix. labels Dec 19, 2017
@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Dec 19, 2017
@codecov
Copy link

codecov bot commented Dec 19, 2017

Codecov Report

Merging #7572 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #7572      +/-   ##
============================================
+ Coverage     51.16%   51.16%   +<.01%     
  Complexity    24883    24883              
============================================
  Files          1602     1602              
  Lines         94730    94730              
  Branches       1368     1368              
============================================
+ Hits          48470    48471       +1     
+ Misses        46260    46259       -1
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️

@jancborchardt
Copy link
Member

Good stuff, but why reduce it from 10px which we used basically everywhere to 3px? It looks a bit strange and more like a thick border than a slight shadow.

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Dec 19, 2017

Good stuff, but why reduce it from 10px which we used basically everywhere to 3px? It looks a bit strange and more like a thick border than a slight shadow.

Because we use that in our newer filters ;) See the line I just changed, I thought that we move to that. Otherwise we need to touch a lot more occurrences.

@skjnldsv
Copy link
Member

I disagree! The filter: drop-shadow is far better than the box-shadow as the arrow isn't included in the shadow generation. I'm against the removal of this.

What's up with safari anyway? it should supports filter? https://caniuse.com/#feat=css-filters 😕

@MorrisJobke
Copy link
Member Author

Usually it does this, but somehow not for the notifications drop down. I will check tomorrow and adapt this pr then

@MorrisJobke
Copy link
Member Author

Okay - I moved to the 10px and to the filter: drop-shadow. I also added fixes for IE 11, because this supports only box-shadow. I tested it and it works now 👍

* unify shadow blur from 3px to 10px
* remove opacity of background of app labels
* for IE: use box-shadow as fallback (because the filter: drop-shadow is not supported)

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke merged commit 0b8a9fc into master Dec 20, 2017
@MorrisJobke MorrisJobke deleted the fix-box-shadow branch December 20, 2017 12:52
@MorrisJobke MorrisJobke mentioned this pull request Jan 2, 2018
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants