Skip to content

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jun 29, 2018

This adds a fallback icon for primary colors that are red.

screenshot_2018-06-29 dateien - nextcloud

The fallback is not used on bright redish colors and on to dark colors where the red dot is clearly visible.

fixes #104

@nextcloud/designers

@MariusBluem
Copy link
Member

By testing this PR, I saw we are not showing the notification icon permanently ... is this by intention? I thought we wanted to change this behavior? @nickvergessen

Ref.: #95 (PR), Issue: #2 🤔

@juliusknorr
Copy link
Member Author

@MariusBluem Works fine for me here.

@nickvergessen
Copy link
Member

Hmm, should we really pick white? Or use another color or black for the dot, so it's better spotable?

@MariusBluem
Copy link
Member

We could simply take the opposite color:
b224934e-863f-4264-a13e-69aa21be1537

@juliusknorr
Copy link
Member Author

The issue with a black dot is that it might not be nicely visible on the various brightness levels of red colors we can have there:

image image image

@MariusBluem Complementary colors also don't work nicely in terms of contrast:
image

The white one still seems to be the most obvious one to me, that is clearly visible though all red color values. In cases when the theming color is bright enough, we still use the regular black icon with the red dot, which also looks good then:

image

@nickvergessen
Copy link
Member

Yes, it's "visible" but not "disturbing" enough:
Just look at your screenshot:
image

Do you immediately see that there is a notification?

@juliusknorr
Copy link
Member Author

Do you immediately see that there is a notification?

I'd say yes, since the icon also doesn't have any opacity as the other icons, but I welcome other ideas on how we can make it a more obvious.

@MariusBluem
Copy link
Member

Or we make it flashing ...

@nickvergessen
Copy link
Member

We have a little animation, but only for ~3secs. If you load the app in the background you miss it, anyway... lets continue with being productive

@jancborchardt
Copy link
Member

I’d say it’s a good enhancement already.

In addition, in #104 (comment), @j-ed did a mockup with a blue notification bubble color (we could use Nextcloud blue) which would be used in these cases. Maybe worth a try @juliushaertl?

But I’m also fine with it being just white there, as mentioned the opacity is also different.

@skjnldsv
Copy link
Member

skjnldsv commented Jul 2, 2018

let's make the dot pulse?

@jancborchardt
Copy link
Member

Nothing in the interface should pulse consistently, that's irritating.

We do have the form (dot), we simply need to add the color here too.

@juliusknorr
Copy link
Member Author

Here are some examples with the blue dot, but I also think that this doesn't work well in terms of contrast:

image image image

But I've no strong opinion on this, let's not invest to much time into an edgecase like this. I'd say we should merge the white one for now and we can still improve later if needed.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me with the white dot. :)

.notifications-button.hasNotifications {
background-repeat: no-repeat;
background-position: center center;
background-image: url('../img/notifications-new-fallback.svg?v=1');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file does not exist 🙈

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, we are not using background images, but a normal image.
The path is calculated in

iconPath: function() {
var iconPath = 'notifications';
if (/*this.backgroundFetching &&*/ this.notifications.length) {
iconPath += '-new';
}
if (this.invertedTheme) {
iconPath += '-dark';
}
return OC.imagePath('notifications', iconPath);

Can we do this calculation properly there as well?

@MorrisJobke
Copy link
Member

Beside that I'm fine with the white one 👍

@MorrisJobke
Copy link
Member

I checked out this branch and changed the color to #FF0000 but it still shows the old avatar. Even after clearing the browser cache.

bildschirmfoto 2018-07-03 um 18 05 37

Even after clearing the SCSS caches via occ maintenance:repair it still shows the old one.

@nickvergessen
Copy link
Member

Replaced by #156

@nickvergessen nickvergessen deleted the bugfix/104/red-color-fix branch August 27, 2018 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

automatic color adjustments should take place based on the selected theme color

7 participants